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

feat: add flag for forced shadow migrate mode #3894

Merged
merged 12 commits into from
Dec 11, 2023

Conversation

nolanlawson
Copy link
Contributor

Details

This adds a new runtime flag, ENABLE_FORCE_SHADOW_MIGRATE_MODE, that, when enabled, forces components that don't have static 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?

  • ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • ✅ No, it does not introduce an observable change.

@nolanlawson nolanlawson requested a review from a team as a code owner December 8, 2023 23:00
import StyledLight from 'x/styledLight';

function doubleMicrotask() {
// wait for applyShadowMigrateMode()
Copy link
Contributor

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

Copy link
Contributor Author

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 () => {
Copy link
Contributor

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();
Copy link
Contributor

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?

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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@ravijayaramappa ravijayaramappa left a 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) {
Copy link
Member

@jmsjtu jmsjtu Dec 11, 2023

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nolanlawson nolanlawson merged commit c3e294b into master Dec 11, 2023
9 checks passed
@nolanlawson nolanlawson deleted the nolan/force-shadow-migrate-mode branch December 11, 2023 22:17
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