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

RFC: Shadow DOM migration mode #83

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

nolanlawson
Copy link
Contributor

However, this technique was already tested on a non-trivial internal Salesforce app, and there was only one component with incorrect styles. Therefore, "migrate" mode can be thought of as a "best effort" to emulate the behavior of synthetic shadow DOM, which will probably get component authors 99.9% of the way there, but without the effort required to _fully_ migrate to native shadow.

Also, it is understood that copying styles into shadow roots may lead to a performance cost. However, browsers already have
[optimizations for repeated stylesheets in shadow roots](https://github.com/whatwg/dom/issues/831#issuecomment-585489960), so this cost is not expected to be large.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking over the comment from Emilio, this optimization applies to repeated <style>s, not repeated <link>s (necessarily). We should write a benchmark to confirm that <link rel=stylesheet>s, when duplicated in a shadow root, do not run into a performance cliff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My current implementation actually uses constructable stylesheets, so this may be a non-issue actually.


> Switch your components into "migrate" mode and experience improved performance with minimal breaking changes.

We could also potentially leverage [lwc-codemod](https://github.com/salesforce/lwc-codemod) and/or IDE prompts to encourage adoption.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please!


Finally, `'migrate'` mode does not attempt to emulate non-stylesheet differences between native and synthetic shadow. For example, it does not attempt to emulate "lazy" slots, or the nonstandard behavior of `innerHTML`, or other quirks. These are all things that `'migrate'` _could_ do in a subsequent version, possibly gated by [component-level API versioning](https://lwc.dev/guide/versioning#component-level-api-versioning).

For a v1 of `'migrate'` mode, however, merely cloning stylesheets should provide the most bang for the buck. It is not a complicated feature to implement, and it is likely to cover the biggest hurdle that component authors would hit when migrating from synthetic to native shadow.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is the cloning done?

Are changes to the head styling applied after cloning copied into components?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the PoC (salesforce/lwc#3894), we use a MutationObserver to keep the copied styles up to date, so yes. This is necessary for things like Aura components that may render global styles on-demand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should do that... I think we should just catch whatever has been inserted before LWC boots, or time to first component creation. If you're inserting styles arbitrarily, presumably from a component code, I don't think we should support that... I understand that the whole point here is to get us rolling on the migration, but my problem with using MO is that it is extremely difficult to know why your component is not working under certain conditions, because the conditions cannot be simply assess.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another big issue is what will happen to all components with migrate if someone inserts a new style at the top?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another big issue is what will happen to all components with migrate if someone inserts a new style at the top?

The components get the new styles.

The current PoC is very aggressive, since we don't know if someone is adding dynamic Aura components that inject global styles. We can wind it down if it ends up being a perf concern. This is just a PoC. 🙂

The downside is, of course, an ugly load experience and potential hit to [Cumulative Layout Shift](https://web.dev/articles/cls). The solution
for component authors who are concerned about this is to move fully from `'migrate'` to `'native'` shadow mode.

While it would be technically possible to have a handoff between the client and server where both are aware of which `<link>` and `<style>`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand on what makes this complex?

Why can't a call to renderComponent() include passing the list of styles to clone into components?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renderComponent is currently not aware of any global styles on the page. Also, there are a number of sources for global styles:

  1. Stylesheets owned by the page container
  2. Aura components
  3. Styles rendered in another framework (e.g. Vue/Svelte/React components appending <style>s to the <head> as they render client-side – similar to Aura).

The first category would be the easiest to cover, but I'm not sure it's worth it. For a v1 anyway, it's much simpler to say "this is CSR-only."

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FOUC (or mistyled?) is the thing I'd like to avoid, if possible.

In the case of SSR, is it unfair to put the onus on the invoker of renderComponent to provide the right list of style/link tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is totally possible to do, but I would prefer to avoid it for a v1. If it's just SLDS it's pretty easy, but some LWR sites do have custom CSS added via static resources. So to make it work, there would be a lot of moving parts.

Right now my main goal is to validate this approach for the simple case (CSR) before moving on to the complex case (SSR+hydration).

Copy link
Contributor

@caridy caridy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very supportive of this proposal. I do want to see more details about what is going to be copied, and when?

@nolanlawson
Copy link
Contributor Author

nolanlawson commented Jan 25, 2024

I'm very supportive of this proposal. I do want to see more details about what is going to be copied, and when?

@caridy Right now, the PoC (salesforce/lwc#3894) just uses a single constructable stylesheet object, injected into each shadow root once, which is then updated whenever the <head> changes by concatenating all the styles into it.

We can revisit this to try to find the right perf balance – I haven't done extensive benchmarking because I'm just trying to prove the concept.

@@ -151,3 +151,4 @@ We could also potentially leverage [lwc-codemod](https://github.com/salesforce/l

- Do any non-stylesheet behaviors of synthetic shadow DOM need to be emulated?
- How should component-level API versioning apply to this, if at all? E.g., should the "forced" opt-in be done gradually, only for newer API versions?
- Should we have a separate rollout strategy for environments where we want to forcibly remove synthetic shadow DOM? For example, a global "forced" migrate mode could simply assume that any component without `static shadowSupportMode = 'native'` should run in migrate mode. This would allow for removing synthetic shadow from the page entirely.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sympathetic with this approach. I do think that "migrate" is the wrong term, and "forced" is also the wrong term. I think this paragraph will have us to reflect on the nature of this feature better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with whatever name we want. People seem to really like "synthetic synthetic shadow."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at the core functionality of this rather than looking at the problem it solves, this is about styles... specifically global styles being propagated into the shadow. Based on that, something like 'native-with-global-styles' seems very odd and long... perhaps something like 'style-able'. Another alternatives is to never leak this value to the client, basically, it is not observable from code, in which case we don't need a name for it. The question is, when would someone manually use that value in their code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternatives is to never leak this value to the client

Yes, this is the question of opt-in vs opt-out. This PR assumes opt-in since the context is assumed to be a B2B app like LEX.

The question is, when would someone manually use that value in their code?

Per this PR, the answer is "when the component author wants to migrate to native shadow DOM but cannot due to breaking changes due to a reliance on synthetic shadow DOM style leakage."

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.

None yet

3 participants