-
Notifications
You must be signed in to change notification settings - Fork 392
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
feat: add flag for forced shadow migrate mode #3894
Conversation
import StyledLight from 'x/styledLight'; | ||
|
||
function doubleMicrotask() { | ||
// wait for applyShadowMigrateMode() |
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 double wait is due to the usage of MutationObserver? I ask because the lwc:dom tests work with one microtask: packages/@lwc/integration-karma/test/dom-manual/moving-node-to-document/index.spec.js
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, this is due to applyShadowMigrateMode
. TBH I didn't look super deep into why it's two microtasks rather than just one.
); | ||
}); | ||
|
||
it('local styles are defined after global styles', async () => { |
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.
👌
|
||
export function applyShadowMigrateMode(shadowRoot: ShadowRoot) { | ||
if (!globalStylesheet) { | ||
globalStylesheet = initGlobalStylesheet(); |
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 does not have to be eager because, the styles are not needed until the first LWC element is ready to use it. And the mutation observer's callback queries for all stylesheets not added by lwc and inserts them. So stylesheets added before the MO was initialized will also be copied over.
Question: What if the stylesheets are added to the document.head after doAttachShadow
, these don't get copied over until the next time the component is reinstantiated. If it gets rehydrated, it still won't get the styles. Correct?
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 does not have to be eager because, the styles are not needed until the first LWC element is ready to use it.
This is true, but it seems to me it would complicate the code to wait until the precise moment an LWC component needs the styles, which is after renderedCallback
. Here we are doing it at connectedCallback
(which is when attachShadow
is called), which is right before renderedCallback
, so there's really not much difference.
So stylesheets added before the MO was initialized will also be copied over.
Yes, this is a goal – it should copy e.g. a <link rel=stylesheet src=app.css>
that already existed in the HTML even before JS ran.
What if the stylesheets are added to the document.head after doAttachShadow, these don't get copied over until the next time the component is reinstantiated. If it gets rehydrated, it still won't get the styles. Correct?
I'm using a single constructable stylesheet which is copied into every shadow root. Whenever we call replaceSync
, it automatically updates everywhere that that constructable stylesheet is used. I can add a comment about this.
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.
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.
Just a few questions for my own education, otherwise LGTM.
const elms = document.head.querySelectorAll( | ||
'style:not([data-rendered-by-lwc]),link[rel="stylesheet"]' | ||
); | ||
if (elms.length === lastSeenLength) { |
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'm not sure if this would happen or if it's a concern but if someone updates an element for example:
<link rel=stylesheet src=app.css>
-> <link rel=stylesheet src=app2.css>
The length of the elms
would be the same and the update would be skipped.
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.
Yep, good point! I doubt we will need to handle that case, though. I think in most cases people are just adding/removing link tags. But I can add a comment about this.
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.
Details
This adds a new runtime flag,
ENABLE_FORCE_SHADOW_MIGRATE_MODE
, that, when enabled, forces components that don't havestatic shadowSupportMode = 'native'
into a special "migrate" mode where styles are copied from the global<head>
.The goal here is to have a PoC of what a real shadow DOM migration mode might look like, but without having an explicit opt-in for component authors.
If the flag is not enabled, then there should be no observable changes.
Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?