Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multiple independent compiled apps on same browser page: a platform use case w/ PR #7026

Closed
typhonrt opened this issue Dec 18, 2021 · 5 comments · Fixed by #7114
Closed
Labels
bug runtime Changes relating to runtime APIs

Comments

@typhonrt
Copy link
Contributor

typhonrt commented Dec 18, 2021

Describe the problem

I am releasing a significant Svelte component library for Foundry VTT. Foundry VTT is a virtual tabletop for role playing games with an open 3rd party developer API. A couple thousand developers are producing module / game system additions to Foundry and Svelte definitely has a place to bring a considerably better UI development experience. For Svelte to work well on this platform multiple independent compiled Svelte apps need to run on the same browser page.

The general problem is that presently there is a small conflict regarding styles / transitions in the Svelte internal runtime in ./src/runtime/internal/style_manager.ts. Because the keys to store the generated styles / keyframe animation are not unique between runtimes one Svelte app can clobber the stored styles of another if both have transitioning components at the same time. The first runtime to finish all transitions wipes out all stored animation transitions for all runtimes / apps that have components transitioning at the time.

I think it is a big value add for multiple independent / compiled Svelte apps to be able to run on the same browser page. Again this is mostly a platform use case / scenario where many developers could be creating independent apps running at the same time which is the case with Foundry VTT.

Describe the proposed solution

I have made a proof of concept fix, but would like to discuss with the maintainers the appropriate course of action since this is my first potential commit to Svelte and this is a change to the internals. It is not a major change though and all tests pass and no new tests likely need to be written seemingly.

The ExtendedDoc interface needs to be updated for dynamic keys. I'm not a TS expert so in my proof of concept implementation I used the following which is not great, but this interface is not exported and is barely used in this source file:

interface ExtendedDoc extends Document {
	[key: string]: any;
}

So the above is the first point of guidance I'd like to receive from the Svelte maintainers. This would be a great application for RegExp types, but that is not a thing for TS yet.

The next aspect is generating a unique ID. This only has to be defined once per runtime / app. For the PoC I appended Date.now(). This "should" be OK, but a UUID would be better. I didn't see any sort of ID generating function in ./src/runtime/internal/utils.ts, so guidance from the maintainers for suitable ID generation is great.

const uniqueId = Date.now();
const svelteStylesheet = `__svelte_stylesheet_${uniqueId}`;
const svelteRules = `__svelte_rules_${uniqueId}`;

Instead of accessing the stored styles like doc.__svelte_stylesheet use the keys above and access like
doc[svelteStylesheet] and doc[svelteRules] in create_rule and clear_rules.

That's it.. This is IMHO a low impact / high reward change and allows multiple independently compiled Svelte apps to run on the same browser page without conflicts. I couldn't detect any other conflicts in my Svelte lib for Foundry VTT between multiple independent apps.

The full PoC is here, but I'll redo the fork / make a PR w/ discussion and guidance from the core maintainers resulting from the above. Pinging @tanhauhau as you were the last one to touch style_manager.ts... Great tutorial vids BTW!

Also, I have a temporary repo here where I debugged things that goes through an exhaustive description with picts of the problem in the README.

Alternatives considered

There are no alternatives. This is a blocking issue for Svelte apps / multiple runtimes to function correctly on the same browser page. This is valuable for using Svelte for platform use cases where multiple developers release many packages / apps that can be installed simultaneously. While I label this as "blocking" it is not a crashing type scenario, but a problem nonetheless for quality.

Importance

blocking / not crashing / quality improvement

@dummdidumm dummdidumm added bug compiler Changes relating to the compiler labels Dec 18, 2021
@dummdidumm
Copy link
Member

Not well-versed with the internals but this sounds like a sensible change. Could there be anyone relying on the current access path being hardcoded?

@typhonrt
Copy link
Contributor Author

typhonrt commented Dec 18, 2021

Could there be anyone relying on the current access path being hardcoded?

Hi @dummdidumm. I don't believe so as if you search the entire codebase for __svelte it is a similar pattern for internal data storage. There are no other usages of __svelte_stylesheet and __svelte_rules elsewhere in the codebase and this isn't user facing functionality. The styles manager is generating animation / keyframe styles and appending them to the document and then clearing them when all transitions are complete hence the need for unique storage keys per runtime on the same page.

I'm definitely a bit new around here, so do want to receive a little input to make a successful PR out of the gate in a clean manner.

I'd probably switch the tags for this issue from compiler to runtime when you get a chance.

While I marked this issue as blocking it would be great to get it solved and available in a months time. It isn't a crashing bug, but a visually disruptive one for the use case I presented.

@dummdidumm dummdidumm added runtime Changes relating to runtime APIs and removed compiler Changes relating to the compiler labels Dec 18, 2021
@typhonrt
Copy link
Contributor Author

typhonrt commented Jan 5, 2022

@dummdidumm Could you possibly put me in touch with the relevant core maintainers to have a discussion about the above? I'd like to make a PR that is in line with all expectations and that will be addressed and merged for an upcoming release. The fix is small and the outcome is large for this particular application regarding quality and feasibility for the use case mentioned.

I definitely wanted to wait until the new year to give any maintainers a needed break over the holidays. However, this is a pressing issue for the platform use case / Svelte based runtime / UI library I'm releasing imminently. I've put a lot of work into this effort so want to get this quality blocker taken care of ASAP.

@benmccann
Copy link
Member

There's a lot of text in the description, so to summarize: the problem is that when a transition is finished running it clears all the temporary style rules for the transition. When there are multiple Svelte apps on a page that ends up clearing the style rules for all Svelte apps

I don't know if it's better to have one stylesheet per app like the proof-of-concept or to try to be more specific about clearing just the relevant rules. But I agree with the general goal and the code you have mostly looks good (it should be switched to snake_case), so I'd say go ahead and send a PR and then someone with more knowledge of this section can give a final approval

@typhonrt
Copy link
Contributor Author

typhonrt commented Jan 6, 2022

Thanks for our chat @benmccann on the Svelte Discord. Indeed you summarized things well enough. style_manager is storing transitions for any Svelte runtime under the same static key so when multiple app / runtimes are running transitions simultaneously the first runtime to finish clears out all transition styles for all runtimes on the same browser page. This causes all sorts of visual glitches when two or more apps / Svelte runtimes have components transitioning at the same time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug runtime Changes relating to runtime APIs
Projects
None yet
3 participants