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

Declarative Shadow DOM #38

Closed
dgp1130 opened this issue Jun 26, 2021 · 5 comments
Closed

Declarative Shadow DOM #38

dgp1130 opened this issue Jun 26, 2021 · 5 comments
Assignees
Labels
feature New feature or request

Comments

@dgp1130
Copy link
Owner

dgp1130 commented Jun 26, 2021

I've been struggling lately with a lot of CSS bundling and scoping challenges (see #37). With Declarative Shadow DOM on its way and already supported by Chrome, I think a better strategy might be to find a DX story around that and then find a way to gracefully fall back for browsers that don't support it,

I experimented with this in ref/declarative-shadow-dom-prototype and got something which reasonably works. It roughly looks like this:

export async function renderComponent(lightDom: string): Promise<string> {
    return `
        <!-- Host element. -->
        <div id="component">
            <!-- Shadow root. -->
            <template shadowroot="open">
                <!-- Polyfill declarative shadow DOM for browsers that don't support it yet. -->
                ${polyfillDeclarativeShadowDom()}

                <!-- Inline styles to apply them to the shadow root. -->
                ${await inlineStyle('rules_prerender/examples/declarative_shadow_dom/component.css')}

                <!-- Shadow DOM content, styled with the associated style sheet. -->
                <div>Shadow content</div>

                <!-- Slot to insert the associated light DOM. -->
                <slot></slot>
            </template>

            <!-- Light DOM content, not affected by above style sheet. -->
            ${lightDom}
        </div>
    `;
}

The new inlineStyle() function is similar to includeStyle() but instead of injecting an HTML comment which gets postprocessed into a singular <style /> tag for the page, it gets directly inlined in its own <style /> tag at the specified location. This allows users to put styles directly inside a shadow root, which otherwise wouldn't be possible without hard-coding the CSS. This scopes the CSS to only the DOM elements directly in the shadow DOM.

I also needed to add a polyfill for browsers which don't support it. This seems to work ok, but does require JavaScript, likely has a FOUC, and needs to be manually included anywhere a declarative shadow root is used.

This mostly seems to work and solves the expected problem, but there are a few open questions to resolve:

I suspect the implementation might still be a little buggy (unfortunately declarative shadow DOM doesn't work in Stackblitz so its hard to make a minimal reproduction for these):

  • I can't get a declarative shadow root with an unnamed <slot /> element with default content to actually load. It works if I use a named <slot /> with default content, but I'm not sure why the name is required. This might be a quirk of the spec for complicated reasons or just a bug in Chrome's implementation.
  • After adding about 100 shadow roots, each of which inlining a 100 line CSS file, the polyfill breaks in Chrome for some reason. It seems like the last shadow root doesn't get parsed correctly and doesn't get transformed into a real shadow root, then gets picked up by the polyfill, which fails. I couldn't find anything wrong with the syntax or anything unique about the last element. I'm also not sure why I need so many shadow roots before this bug reproduces. Definitely seems like a bug here.

I'm not sure about the best way to load CSS. AFAICT, the only way to apply CSS inside a declarative shadow root is to have a <style /> or <link /> tag within it. I don't want to use a <link /> tag because it means I need to bundle each component's shadow CSS in a separate file and serve it (which is very awkward and rules_prerender doesn't take that much liberty with file structure atm, the user is supposed to be in control of that). I also don't want to use inline a <style /> tag because it will be repeated every time an element is rendered, significantly increasing the bundle size.

For now I'm just inlining the <style /> tag each time. I was hoping that gzip would do a good job of compressing such highly repetitive content but my initial experiments aren't very promising. Even if the bundle size increase is compressed out, the styles still need to be parsed multiple times, take up extra memory, and aren't as easy to debug (modifying one component's styles in DevTools wouldn't affect other components of the same type).

What I would love to see is something like <style shadow-id="foo" />, then declare a shadow root with <template shadowroot="open" styles="foo" /> to reference the specific inline style that should be applied to that root. I suppose that's possible via runtime JavaScript, but its not built into the standard. I'll need to do more investigation and exploration here to understand what is the best way of loading styles in a repeating declarative shadow DOM structure.

One other question is about tree shaking CSS. This model provides no means of tree shaking unused styles. We could add PurgeCSS and run it on the output, though I'm not sure if it's smart enough to take declarative shadow DOM into account. Even if it does, we always run the risk of client-side JS applying a class at runtime when the style got erased. That's kind of an independent issue, but I want to make sure that whatever strategy we use for CSS is tree shakable, as that will undoubtedly be a necessary optimization.

@dgp1130 dgp1130 added the feature New feature or request label Jun 26, 2021
@dgp1130 dgp1130 mentioned this issue Jun 26, 2021
dgp1130 added a commit that referenced this issue Jul 24, 2021
Refs #38.

This includes an `inlineStyle()` function to inline a full style block into a shadow root (necessary to apply the styles) and a polyfill for declarative shadow DOM.
dgp1130 added a commit that referenced this issue Jul 24, 2021
Refs #38.

The `inlineStyle()` function simply loads the given input from runfiles and returns it in a `<style />` element. This is useful for inlining styles in specific parts of the document, such as inside a declarative shadow DOM template.
dgp1130 added a commit that referenced this issue Jul 24, 2021
@dgp1130
Copy link
Owner Author

dgp1130 commented Jul 25, 2021

I've been continuing to iterate on this and have something that I think is usable. Unfortunately I'm struggling a bit with tests. I would like to make some automated test for the declarative shadow DOM polyfill, but I don't have a set browser test setup just yet. I tried integrating Karma but after a few hours I'm declaring miserable failure.

I immediately ran into bazelbuild/rules_nodejs#2093 and had to upgrade rules_nodejs (and @bazel/*) to >= 3.4.2 to get a fix. This seems to break some other targets, but I haven't gotten to them yet.

After that, I got some errors like this which cause a timeout after 30 seconds:

24 07 2021 21:48:46.074:WARN [web-server]: 404: /jasmine.js
Chrome Headless 76.0.3809.0 (Linux x86_64) ERROR: 'There is no timestamp for jasmine.js!'

Which led me to bazelbuild/rules_nodejs#1872 and bazelbuild/rules_nodejs#1867, I don't fully understand everything, but it seems like the files need to be UMD bundles, which doesn't seem to be the case here.

I tried to fix this by using a tsconfig with module: "umd" and introducing a new js_library() with named_module_srcs (which appears to be undocumented, but is used in the Karma example?)

This is able to actually run the test, however it executes 0 of 0 specs. So the files aren't being loaded correctly for some reason. I made sure the files include an import / export so the files are interpreted as modules. I also added /// <amd-module name="..." /> comments as used in the example. However the test still doesn't execute anything.

Running the test in my own browser and opening DevTools, I see a temporary file with this content:

      // A simplified version of Karma's requirejs.config.tpl.js for use with Karma under Bazel.
      // This does an explicit `require` on each test script in the files, otherwise nothing will be loaded.
      (function(){
        var runtimeFiles = [
          // BEGIN RUNTIME FILES
          
          // END RUNTIME FILES
        ].map(function(file) { return file.replace(/\.js$/, ''); });
        var allFiles = [
            // BEGIN USER FILES
            
            // END USER FILES
        ];
        var allTestFiles = [];
        allFiles.forEach(function (file) {
          if (/[^a-zA-Z0-9](spec|test)\.js$/i.test(file) && !/\/node_modules\//.test(file)) {
            allTestFiles.push(file.replace(/\.js$/, ''))
          }
        });
        require(runtimeFiles, function() { return require(allTestFiles, window.__karma__.start); });
      })();

Note the // BEGIN USER FILES and // END USER FILES with no files between them. It definitely looks like my tests should be included in there, but they just aren't for no particular reason.

It's worth noting this quote from the @bazel/concatjs documentation:

Ultimately by using concatjs, you’re signing up for at least a superficial understanding of [UMD].

I definitely do not have that much understanding of UMD and don't particularly care to learn it just to get some Karma tests to work.

My prototype is in ref/karma.

@dgp1130
Copy link
Owner Author

dgp1130 commented Jul 25, 2021

After making no progress with Karma, I went looking to see if I could run browser tests any other way in Bazel. Jest has some support, but I'm personally not a fan of running browser tests on a fake environment built on Node. I would rather use a real browser if possible.

I took another look at @web/test-runner, which uses a real browser to test with ESM support and a lot less legacy than Karma or the karma_web_test_suite() Starlark rules. I couldn't find anyone who has actually tried to use @web/test-runner in Bazel, but I took a stab at it just to see if it would be possible. After a bit of fiddling, I was actually able to get something working, see ref/web-test-runner.

I'm not convinced that it's a good idea to use custom infrastructure here, but maybe it would be worth making a separate project to support @web/test-runner on Bazel? I'll keep playing around and thinking about whether or not I want to move forward with this.

dgp1130 added a commit that referenced this issue Jul 26, 2021
dgp1130 added a commit that referenced this issue Jul 26, 2021
Refs #38.

The `inlineStyle()` function simply loads the given input from runfiles and returns it in a `<style />` element. This is useful for inlining styles in specific parts of the document, such as inside a declarative shadow DOM template.
dgp1130 added a commit that referenced this issue Jul 26, 2021
@dgp1130
Copy link
Owner Author

dgp1130 commented Jul 26, 2021

I've been skipping the testing question for now as I don't think browser test infra would fully solve the problem here. I can't easily test the Declarative Shadow DOM polyfill anyways because it needs to run on an HTML page which has templates that were not processed and converted into shadow roots. That means I'd need to test on an old browser, and with a specific HTML file. While this probably possible, I'm not sure it's worth the effort to set up anyways.

Regardless, I've been stuck for a while trying to make the Declarative Shadow DOM component publishable to NPM (see #39). It is necessary to publish a component because users need to include the client-side script based on possibly conditional logic in their prerender code. I was planning to make something simple by publishing the TypeScript source code and letting users recompile it on demand. I was able to make this work within Bazel after tackling a few unexpected but related issues (alias() of a prerender_component() requires some more effort, and we need to publish the tsconfig.json the component is compiled with). However I'm not able to figure out the best way to import the component just yet. It can't be in the regular import { polyfillDeclarativeShadowDom } from rules_prerender, because it is built as a prerender_component(), not a regular ts_library(). If we allowed that import, the client side script would get dropped.

Instead, users have to depend upon a prerender_component() at @npm//rules_prerender:declarative_shadow_dom. That much works, but the big question is how to import it. The file is actually at packages/rules_prerender/declarative_shadow_dom.ts, which is a private path. It's also TypeScript, so the actual compiled JS is actually at:

dist/bin/out/k8-opt-exec-2B5CBBC6/bin/external/npm/rules_prerender/packages/rules_prerender/declarative_shadow_dom.js

That means we need some reasonable import statement to resolve to that path both in NodeJS at runtime, and in TypeScript (and tsserver in the IDE). I'm not really sure how to make either of those work. Adding to the complexity here is the fact that this path is in a non-standard configuration (2B5CBBC6). This value is not consistent and will vary based on user code, so we can't hard-code anything here in source.

I'm starting the think this "ship TS to NPM and compile it on the user's machine" isn't much of a workaround. It might be easier to just address the core problem and find a way to create a prerender_component() from JS by fixing #39.

dgp1130 added a commit that referenced this issue Aug 15, 2021
Refs #38, #39, #4.

This allows JS source files to be included in `prerender_component()` and related rules, both for prerendering HTML and as client side scripts. This mostly involved replacing the relevant `ts_library()` targets with `js_library()` targets to compile user code and avoid depending on it from a `ts_library()` which would require `*.d.ts` definitions that the user may not write.

I included support for `*.mjs` and `*.cjs` files, although I have had trouble getting ESM to work with rules_nodejs and `*.mjs` doesn't work for a lot of complicated reasons not related to `prerender_component()`.

Since we need to compile user code with either `ts_library()` or `js_library()`, it means that a given `prerender_component()` target can only compile *all* TypeScript or *all* JavaScript. This is an unfortunate requirement, but not also a reasonable ask of users, so I don't feel too bad for not supporting it.
dgp1130 added a commit that referenced this issue Aug 15, 2021
Refs #38, #39, #4.

This example application only uses `*.js` source files to validate the use case. It includes JavaScript prerendering code, both in a `prerender_page()` and a standalone `prerender_component()`. It also includes JS client-side scripts and validates that they get included in the final bundle *and* are properly tree shaken when not used.
dgp1130 added a commit that referenced this issue Aug 15, 2021
Refs #38, #39, #4.

This example application only uses `*.js` source files to validate the use case. It includes JavaScript prerendering code, both in a `prerender_page()` and a standalone `prerender_component()`. It also includes JS client-side scripts and validates that they get included in the final bundle *and* are properly tree shaken when not used.
dgp1130 added a commit that referenced this issue Aug 28, 2021
Refs #38, #39, #4.

This example application only uses `*.js` source files to validate the use case. It includes JavaScript prerendering code, both in a `prerender_page()` and a standalone `prerender_component()`. It also includes JS client-side scripts and validates that they get included in the final bundle *and* are properly tree shaken when not used.
dgp1130 added a commit that referenced this issue Aug 28, 2021
Refs #38, #39, #4.

This example application only uses `*.js` source files to validate the use case. It includes JavaScript prerendering code, both in a `prerender_page()` and a standalone `prerender_component()`. It also includes JS client-side scripts and validates that they get included in the final bundle *and* are properly tree shaken when not used.
dgp1130 added a commit that referenced this issue Aug 28, 2021
Refs #38, #39, #4.

This example application only uses `*.js` source files to validate the use case. It includes JavaScript prerendering code, both in a `prerender_page()` and a standalone `prerender_component()`. It also includes JS client-side scripts and validates that they get included in the final bundle *and* are properly tree shaken when not used.
dgp1130 added a commit that referenced this issue Aug 28, 2021
Refs #38, #39, #4.

This example application only uses `*.js` source files to validate the use case. It includes JavaScript prerendering code, both in a `prerender_page()` and a standalone `prerender_component()`. It also includes JS client-side scripts and validates that they get included in the final bundle *and* are properly tree shaken when not used.
dgp1130 added a commit that referenced this issue Aug 28, 2021
Refs #38, #39, #4.

This example application only uses `*.js` source files to validate the use case. It includes JavaScript prerendering code, both in a `prerender_page()` and a standalone `prerender_component()`. It also includes JS client-side scripts and validates that they get included in the final bundle *and* are properly tree shaken when not used.
dgp1130 added a commit that referenced this issue Aug 28, 2021
Refs #38, #39, #4.

This example application only uses `*.js` source files to validate the use case. It includes JavaScript prerendering code, both in a `prerender_page()` and a standalone `prerender_component()`. It also includes JS client-side scripts and validates that they get included in the final bundle *and* are properly tree shaken when not used.
dgp1130 added a commit that referenced this issue Aug 28, 2021
Refs #38, #39, #4.

This example application only uses `*.js` source files to validate the use case. It includes JavaScript prerendering code, both in a `prerender_page()` and a standalone `prerender_component()`. It also includes JS client-side scripts and validates that they get included in the final bundle *and* are properly tree shaken when not used.
dgp1130 added a commit that referenced this issue Aug 29, 2021
Refs #38.

The `inlineStyle()` function simply loads the given input from runfiles and returns it in a `<style />` element. This is useful for inlining styles in specific parts of the document, such as inside a declarative shadow DOM template.
dgp1130 added a commit that referenced this issue Aug 29, 2021
dgp1130 added a commit that referenced this issue Aug 29, 2021
Refs #38.

The `inlineStyle()` function simply loads the given input from runfiles and returns it in a `<style />` element. This is useful for inlining styles in specific parts of the document, such as inside a declarative shadow DOM template.
dgp1130 added a commit that referenced this issue Aug 29, 2021
dgp1130 added a commit that referenced this issue Sep 5, 2021
Refs #38.

The `inlineStyle()` function simply loads the given input from runfiles and returns it in a `<style />` element. This is useful for inlining styles in specific parts of the document, such as inside a declarative shadow DOM template.
dgp1130 added a commit that referenced this issue Sep 5, 2021
dgp1130 added a commit that referenced this issue Sep 5, 2021
Refs #38.

The `inlineStyle()` function simply loads the given input from runfiles and returns it in a `<style />` element. This is useful for inlining styles in specific parts of the document, such as inside a declarative shadow DOM template.
dgp1130 added a commit that referenced this issue Sep 5, 2021
dgp1130 added a commit that referenced this issue Sep 5, 2021
dgp1130 added a commit that referenced this issue Sep 5, 2021
Refs #38.

The `inlineStyle()` function simply loads the given input from runfiles and returns it in a `<style />` element. This is useful for inlining styles in specific parts of the document, such as inside a declarative shadow DOM template.
dgp1130 added a commit that referenced this issue Sep 5, 2021
@dgp1130
Copy link
Owner Author

dgp1130 commented Sep 5, 2021

I was finally able to solve the publishing problem in #39 and took another stab at this. I was able to develop a prototype which correctly publishes the full Declarative Shadow DOM component, including the client-side polyfill, and link / bundle everything correctly. I'll need to refine it a bit, but I'm pretty confident this can be landed without shaving too much more of a yak.

See the declarative-shadow-dom branch for current progress.

dgp1130 added a commit that referenced this issue Sep 7, 2021
Refs #38.

The `inlineStyle()` function simply loads the given input from runfiles and returns it in a `<style />` element. This is useful for inlining styles in specific parts of the document, such as inside a Declarative Shadow DOM template.

Since this function directly reads the CSS file and returns its contents, it must be `async`, which can be awkward an unintuitive to use. I expect most users will do `<div>${inlineStyle('wksp/foo.css')}</div>`, but this would actually print `<div>[object Promise]</div>` becuase the user didn't `await`. Unfortunately neither TypeScript nor ESLint seem to catch this mistake out of the box, and I expect it will be a common problem. For now, this is good enough to unblock Declarative Shadow DOM, however in the future it would be a good idea to make this synchronous and leave a style annotation which is processed later to actually inline the style. Doing so leads to a bunch of other questions that I don't want to get into right now.

1. Is inlining done by the annotation extractor or resource injector?
2. If done by the resource injector, how do we keep things deterministic? Is it weird that annotation extractor actually leaves annotations for a future tool to process?
3. How do we enforce strict deps?
4. Should we allow users to resolve package-local files?
dgp1130 added a commit that referenced this issue Sep 7, 2021
Refs #38.

This component simply includes the Declarative Shadow DOM polyfill. I left out tests for the polyfill because we don't have a Karma/browser test setup yet and even then it may be difficult to test since the browser would automatically processes any declared shadow roots. It may be possible to work around, but that's something to deal with later.
dgp1130 added a commit that referenced this issue Sep 7, 2021
Refs #38.

This component simply includes the Declarative Shadow DOM polyfill in the resulting JavaScript bundle.

I left out tests for the polyfill because we don't have a Karma/browser test setup yet and even then it may be difficult to test since the browser would automatically processes any declared shadow roots. It may be possible to work around, but that's something to deal with later.

Most of the complexity here is around making the component publishable to NPM. This requires an additional component at the root of the workspace just to re-export the real implementation so users can import it at `rules_prerender/declarative_shadow_dom`. See [this issue](#39 (comment)) for more information on why publishing a component looks the way it does.

One awkward aspect is that there are technically two components here (`//packages/rules_prerender/declarative_shadow_dom` and `//:declarative_shadow_dom`), the former is the real implementation and the latter is the re-export. This means that both components need to be published, but both also need hand-written `BUILD.publish` files. As a result, we use `prerender_component_publish_files()` twice, even though it is a bit redundant to have two and they heavily overlap. Regardless, I think it makes more sense for managing the `BUILD.publish` files, even if one of the `prerender_component_publish_files()` isn't strictly necessary to include.
dgp1130 added a commit that referenced this issue Sep 7, 2021
Refs #38.

This renders an example component using Declarative Shadow DOM and a simple style limited to the shadow root.
dgp1130 added a commit that referenced this issue Sep 7, 2021
Refs #38.

This component simply includes the Declarative Shadow DOM polyfill in the resulting JavaScript bundle.

I left out tests for the polyfill because we don't have a Karma/browser test setup yet and even then it may be difficult to test since the browser would automatically processes any declared shadow roots. It may be possible to work around, but that's something to deal with later.

Most of the complexity here is around making the component publishable to NPM. This requires an additional component at the root of the workspace just to re-export the real implementation so users can import it at `rules_prerender/declarative_shadow_dom`. See [this issue](#39 (comment)) for more information on why publishing a component looks the way it does.

One awkward aspect is that there are technically two components here (`//packages/rules_prerender/declarative_shadow_dom` and `//:declarative_shadow_dom`), the former is the real implementation and the latter is the re-export. This means that both components need to be published, but both also need hand-written `BUILD.publish` files. As a result, we use `prerender_component_publish_files()` twice, even though it is a bit redundant to have two and they heavily overlap. Regardless, I think it makes more sense for managing the `BUILD.publish` files, even if one of the `prerender_component_publish_files()` isn't strictly necessary to include.
dgp1130 added a commit that referenced this issue Sep 7, 2021
Refs #38.

This renders an example component using Declarative Shadow DOM and a simple style limited to the shadow root.
dgp1130 added a commit that referenced this issue Sep 7, 2021
Refs #38.

The `inlineStyle()` function simply loads the given input from runfiles and returns it in a `<style />` element. This is useful for inlining styles in specific parts of the document, such as inside a Declarative Shadow DOM template.

Since this function directly reads the CSS file and returns its contents, it must be `async`, which can be awkward an unintuitive to use. I expect most users will do `<div>${inlineStyle('wksp/foo.css')}</div>`, but this would actually print `<div>[object Promise]</div>` becuase the user didn't `await`. Unfortunately neither TypeScript nor ESLint seem to catch this mistake out of the box, and I expect it will be a common problem. For now, this is good enough to unblock Declarative Shadow DOM, however in the future it would be a good idea to make this synchronous and leave a style annotation which is processed later to actually inline the style. Doing so leads to a bunch of other questions that I don't want to get into right now.

1. Is inlining done by the annotation extractor or resource injector?
2. If done by the resource injector, how do we keep things deterministic? Is it weird that annotation extractor actually leaves annotations for a future tool to process?
3. How do we enforce strict deps?
4. Should we allow users to resolve package-local files?
dgp1130 added a commit that referenced this issue Sep 7, 2021
Refs #38.

This component simply includes the Declarative Shadow DOM polyfill in the resulting JavaScript bundle.

I left out tests for the polyfill because we don't have a Karma/browser test setup yet and even then it may be difficult to test since the browser would automatically processes any declared shadow roots. It may be possible to work around, but that's something to deal with later.

Most of the complexity here is around making the component publishable to NPM. This requires an additional component at the root of the workspace just to re-export the real implementation so users can import it at `rules_prerender/declarative_shadow_dom`. See [this issue](#39 (comment)) for more information on why publishing a component looks the way it does.

One awkward aspect is that there are technically two components here (`//packages/rules_prerender/declarative_shadow_dom` and `//:declarative_shadow_dom`), the former is the real implementation and the latter is the re-export. This means that both components need to be published, but both also need hand-written `BUILD.publish` files. As a result, we use `prerender_component_publish_files()` twice, even though it is a bit redundant to have two and they heavily overlap. Regardless, I think it makes more sense for managing the `BUILD.publish` files, even if one of the `prerender_component_publish_files()` isn't strictly necessary to include.
dgp1130 added a commit that referenced this issue Sep 7, 2021
Refs #38.

This renders an example component using Declarative Shadow DOM and a simple style limited to the shadow root.
@dgp1130
Copy link
Owner Author

dgp1130 commented Sep 7, 2021

Merged Declarative Shadow DOM support to main. Example is here.

This will be included in the 0.0.10 release.

@dgp1130 dgp1130 self-assigned this Sep 7, 2021
@dgp1130 dgp1130 closed this as completed Sep 7, 2021
dgp1130 added a commit that referenced this issue Sep 7, 2021
Refs #38.

Forgot to include this with the initial implementation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant