fix: allow component to be mounted in iframe #13225
Conversation
🦋 Changeset detectedLatest commit: 8026f4f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Thank you for bringing this up!
Now that this PR shows that we need to move the check into the microtask, I'm wondering whether or not the seen checks are even necessary anymore (@Rich-Harris mentioned this here as the main reason) and we should instead just always fall through to the "does a style with the css hash id exist on that target" check. @trueadm thoughts?
I have an idea for the HMR issue, will do a follow-up PR once we decide how to proceed here (or you can add it in here if my following explanation is sufficient). The idea is to have a dev-only map of css -> removal function, i.e. in append_styles you call remove_dev.add(css, () => target.removeChild(style) and hmr calls a new method that calls that function.
Can we check if we're inside an iframe and take a different route? |
|
Is the tiny overhead of possibly unnecessary microtasks really worth the additional complexity/code? It's not the common case anyway, you have to opt in to injected mode, and for custom elements we have to do the microtask either way. |
|
@dummdidumm, Thank you for reviewing this and sorry for prettier issues 😅. About your thought: should instead just always fall through to the "does a style with the css hash id exist on that target" check. @trueadm, the approach is similar for iframes and custom-elements and works on any cases, so I think having a different route is an unnecessary additional complexity. Not? |
|
Svelte 4 did always check the DOM, so I'm not fully convinced that it really makes a difference in real live whether or not we query the DOM |
I think it's a little optimisation but not sure if it's absolutely necessary. Let me know your preference. I implemented your idea of dev-only css cleanup. |
|
This looks good, but I think we need a test to capture the issue otherwise this might regress in the future. |
|
Thanks! Added a test, and made it such that we don't create unnecessary microtasks. Also, we don't need to store cleanup functions, we can just store the |
#13225 did not fully fix the described issue: As soon as you have a child component, that child component's anchor will not be in the right ownerDocument yet, and so the logic falls down. To fix that, we would need to move the ownerDocument check into the microtask - and that map check was mainly done to avoid just that. So instead I opted to simplify the code and just remove the map checks. According to my benchmarking (https://jsbench.me/3hm17l0bxl/1) this has no impact on performance assuming that you'll have cache hits roughly half the time - and even if you'd have much more cache hits, the performance loss is microscopic. Given that the default mode is to not inject styles, this will not be common anyway, so I favor removing that map.
#13225 did not fully fix the described issue: As soon as you have a child component, that child component's anchor will not be in the right ownerDocument yet, and so the logic falls down. To fix that, we would need to move the ownerDocument check into the microtask - and that map check was mainly done to avoid just that. So instead I opted to simplify the code and just remove the map checks. According to my benchmarking (https://jsbench.me/3hm17l0bxl/1) this has no impact on performance assuming that you'll have cache hits roughly half the time - and even if you'd have much more cache hits, the performance loss is microscopic. Given that the default mode is to not inject styles, this will not be common anyway, so I favor removing that map.
|
@Rich-Harris, Thank you for your work, this looks perfect. |
#13225 did not fully fix the described issue: As soon as you have a child component, that child component's anchor will not be in the right ownerDocument yet, and so the logic falls down. To fix that, we would need to move the ownerDocument check into the microtask - and that map check was mainly done to avoid just that. So instead I opted to simplify the code and just remove the map checks. According to my benchmarking (https://jsbench.me/3hm17l0bxl/1) this has no impact on performance assuming that you'll have cache hits roughly half the time - and even if you'd have much more cache hits, the performance loss is microscopic. Given that the default mode is to not inject styles, this will not be common anyway, so I favor removing that map.
4 participants
Hello Svelte Team,
I use Svelte to create injectable widgets in other apps.
I found a good way to do this with svelte by mounting component into an
iframe.This way, it allows a good resource cleanup and good responsive layout.
I used to do this in Svelte 4 and in Svelte 5 prior PR #12374.
In the specified PR, the process of injecting css into dom was changed.
This breaks some cases of mounting a component into an
iframe:This PR enhances
append_stylesto allow it to be aware of the current component context and correctly inject css.It also fix the need to pass
is_custom_elementtoappend_stylessince it's a similar case.HMR Problem:
This PR does not fix the HMR problem.
The HMR problem is that when we cleanup styles (transform-client.js:495), we are outside of the component context so we try to clean styles in the global document.
However, in custom element and iframe contexts, we need to cleanup styles in the context of the mounted component (ie. root for custom element, iframe document for iframe).
Maybe we could keep track of mounted component to ensure we cleanup styles for each components context.
What do you think? Do you have a better solution?
Thanks for watching this and for your amazing work 👍 !