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: light DOM scoped styles #2332

Merged
merged 8 commits into from Sep 15, 2021
Merged

Conversation

nolanlawson
Copy link
Contributor

Details

Rough implementation of this RFC. Adds a *.scoped.css file for light DOM components that results in CSS that is scoped to that component.

Does this PR introduce breaking changes?

  • No, it does not introduce breaking changes.

GUS work item

W-8844642

@nolanlawson nolanlawson force-pushed the nolan/light-dom-scoped-styles-2 branch 3 times, most recently from 3ab8663 to c03a9a8 Compare May 20, 2021 23:54
@nolanlawson nolanlawson force-pushed the nolan/light-dom-scoped-styles-2 branch from 1240317 to b43799b Compare June 11, 2021 21:33
@nolanlawson nolanlawson marked this pull request as ready for review June 11, 2021 22:48
@nolanlawson
Copy link
Contributor Author

Assuming nothing changes in the RFC, this PR is ready for review.

packages/@lwc/compiler/src/options.ts Outdated Show resolved Hide resolved
packages/@lwc/style-compiler/package.json Outdated Show resolved Hide resolved
packages/@lwc/style-compiler/src/transform.ts Outdated Show resolved Hide resolved
packages/@lwc/style-compiler/src/postcss-lwc-plugin.ts Outdated Show resolved Hide resolved
packages/@lwc/rollup-plugin/src/index.js Outdated Show resolved Hide resolved
@@ -97,11 +106,13 @@ enum LWCDOMMode {

export function fallbackElmHook(elm: Element, vnode: VElement) {
Copy link
Member

Choose a reason for hiding this comment

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

We might want to change the name of this function to something more representative. I was originally called this way to encapsulate the fallback logic when the LWC engine is not running with native shadow DOM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, maybe setStyleTokensAndObserve? That seems to be what it's doing, for both synthetic shadow and light DOM scoped styles.

Or I could move the light DOM style scoping to a new hook.

packages/@lwc/engine-core/src/framework/hooks.ts Outdated Show resolved Hide resolved
@nolanlawson nolanlawson force-pushed the nolan/light-dom-scoped-styles-2 branch from 20ec8a2 to 0e6a63b Compare June 24, 2021 20:40
@nolanlawson nolanlawson force-pushed the nolan/light-dom-scoped-styles-2 branch from aeb36a7 to 6ef52fa Compare July 23, 2021 21:15
@nolanlawson nolanlawson force-pushed the nolan/light-dom-scoped-styles-2 branch from 6ef52fa to bddc9c5 Compare September 2, 2021 21:30
@nolanlawson
Copy link
Contributor Author

This PR is ready for a second look. The changes are:

  1. It works for shadow DOM components too.
  2. The scope token is applied client-side rather than server-side.

Copy link
Member

@pmdartus pmdartus left a comment

Choose a reason for hiding this comment

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

Added a couple of nitpicks otherwise the code looks good.

@@ -155,5 +156,5 @@ function normalizeOptions(options: TransformOptions): NormalizedTransformOptions
stylesheetConfig,
outputConfig,
experimentalDynamicComponent,
};
} as NormalizedTransformOptions;
Copy link
Member

Choose a reason for hiding this comment

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

The NormalizedTransformOptions interface should be updated to avoid the type guard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I can't seem to get this to work. TypeScript keeps throwing errors during the build, and I don't understand the problem. Let's address it in a future PR.

return '';
} else if (scopeToken) {
// load the file ourselves without the query param
return fs.readFileSync(filename, 'utf-8');
Copy link
Member

Choose a reason for hiding this comment

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

It is strange that we have to load scoped styles manually from within the plugin while the rest of the files aren't. I understand why this is needed here. However, I am wondering rollup provides a better way to handle this?

Copy link
Member

Choose a reason for hiding this comment

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

Disclaimer I haven't tried this approach. But could we, for example, parse the query string in the resolveId, shelve the query string key/value in the file meta. This would avoid having to add custom logic in the load. The meta can then be retrieved in the transform hook to invoke the compiler correctly.

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 sounds plausible to me; let's handle in another PR? It would need some experimentation.

packages/@lwc/engine-core/src/framework/template.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/template.ts Outdated Show resolved Hide resolved
packages/@lwc/engine-core/src/framework/template.ts Outdated Show resolved Hide resolved
@nolanlawson nolanlawson merged commit 9dfb6a6 into master Sep 15, 2021
@nolanlawson nolanlawson deleted the nolan/light-dom-scoped-styles-2 branch September 15, 2021 17:16
@banderson5144
Copy link

@nolanlawson I am seeing an issue when trying to run a LWR/LWC site. I need to remove ?scoped=true from this line:

https://github.com/salesforce/lwc/blob/master/packages/@lwc/compiler/src/transformers/template.ts#L74

in order to have my LWR/LWC code to function correctly. Is there anything I need to change in my lwr.config.json to make it function correctly:

{
    "lwc": {
        "modules": [
            {
                "dir": "$rootDir/src/modules"
            },
            {
                "npm": "lightning-base-components"
            }
        ]
    },
    "bundleConfig": { "exclude": ["lwc", "@lwc/synthetic-shadow"] },
    "routes": [
        {
            "id": "Home",
            "path": "/",
            "rootComponent": "my/app",
            "layoutTemplate": "$layoutsDir/main_layout.njk",
            "bootstrap": {
                "syntheticShadow": true
            }
        },
        {
            "id": "Foo",
            "path": "/foo",
            "rootComponent": "my/foo",
            "layoutTemplate": "$layoutsDir/main_layout.njk",
            "bootstrap": {
                "syntheticShadow": true
            }
        }
    ]
}

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