Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
RFC: Shadow DOM migration mode #83
Changes from 6 commits
b18e778
b6d3e17
624b959
8a00b3f
7a57af6
f3e2212
ef7906c
99155da
abc6ef3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🙂
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:<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."
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please!