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

[fix] ensure style manager instances don't conflict with each other #7114

Merged
merged 4 commits into from
Jan 11, 2022
Merged

[fix] ensure style manager instances don't conflict with each other #7114

merged 4 commits into from
Jan 11, 2022

Conversation

ivanhofer
Copy link
Contributor

@ivanhofer ivanhofer commented Jan 9, 2022

fixes animations with multiple independent Svelte apps running at the same time.

This is an alternative approach to #7107.

Closes #7107
Fixes #7026

Instead of storing the style information directly on the node via a random property name, here the information get's stored inside a locale varable within the style_manager.

animations with multiple independent Svelte apps running at the same time
@dummdidumm
Copy link
Member

I don't think this works. If two independent instances have the same root (the document), they would still interfere.

@ivanhofer
Copy link
Contributor Author

@dummdidumm each Svelte instance would create it's own stylesheet and it only manages stylesheets that are created within that instance.

@dummdidumm
Copy link
Member

Oh right, got confused here, the map is not shared 👍

@ivanhofer
Copy link
Contributor Author

I first was also confused why ther is a active_docs variable. But is seems it is needed for iframes: #3624

Maybe active_docs isn't a good naming anymore. Maybe rename it to managed_styles?

@dummdidumm
Copy link
Member

I first was also confused why ther is a active_docs variable. But is seems it is needed for iframes: #3624

Good find. I suggest to document this so this is more clear for future readers (including me 😄 )

Maybe active_docs isn't a good naming anymore. Maybe rename it to managed_styles?

👍

@ivanhofer
Copy link
Contributor Author

ivanhofer commented Jan 9, 2022

I just saw that the hash function inside the style_manager uses a slightly different implementation than the hash function inside src/compiler/compile/utils/hash.ts. Do we need two different implementations?

@dummdidumm dummdidumm changed the title fixes #7026 [fix] ensure style manager instances don't conflict with each other Jan 9, 2022
@dummdidumm
Copy link
Member

The hash function in that file is a little larger and it's part of the compiler, not the runtime, so I wouldn't worry about that for now, although I'm not sure if this is just an oversight or a deliberate decision.

@typhonrt
Copy link
Contributor

Looks good.. I hope this gets into the 3.45.1 release..

@tanhauhau
Copy link
Member

i wonder if it is possible to have a test case for this?

@ivanhofer
Copy link
Contributor Author

I will take a look at how to test multiple svelte applications running in parallel. And also a test for the iframe thing should be added. I can try both cases in the next couple of days.
Since both cases are probably unique, does anyone have suggestions how these tests could look like?

@typhonrt
Copy link
Contributor

does anyone have suggestions how these tests could look like?

Yeah that is a tough one as this is a timing / visual glitch. The symptom of when it doesn't work is when two runtimes that have active transitions where the first to finish clears out the style rules of all other runtimes. That may be hard to set up a full test say in puppeteer; I honestly haven't looked at the testing infrastructure deeply, but it looks like puppeteer is involved.

A "synthetic test" could be simulating the call order into style_manager that occurs when transitions run and make sure that one doesn't overwrite the storage of another. I suppose if things can be mocked up in a fashion as well for the DOM elements / styles, etc.

This is definitely in the hard to test / edge case territory.

I did test your PR with my actual runtime environment w/ multiple independently compiled apps to make sure it works. Given that this may become a more common usage pattern on "platform" like efforts any issues would be raised fairly quickly.

@ivanhofer
Copy link
Contributor Author

I just saw that there is already a test for the iframe stuff: transition-css-iframe.

The svelte internals where the style_manager code lives are always loaded from disk, so I'm not quite sure how such a test would work. I think it is not possible with the current test setup.
A new test system is needed for this use case (or another one needs to be extended). Perhaps loading multiple svelte internals can be done via a rollup configuration that bundles the internals for each component separately?
I'm sorry, but I'll have to pass this on to someone else as I don't have the time to investigate this problem in detail.

@benmccann
Copy link
Member

We discussed and agreed a test should not block this given the difficulty

@benmccann benmccann merged commit 806cba2 into sveltejs:master Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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