Postprocess inlined styles

[dgp1130]

Currently, CSS in declarative shadow DOM works via inlineStyle(), which has two critical problems:

1. It directly reads the referenced CSS file from runfiles. This requires I/O, meaning the whole API is async and requires an await. The virality of await means that any users must also await and it becomes a much more annoying API to use than one would expect. On top of this, forgetting to await does not surface as any kind of compile time or run time error:

   console.log(`<div>${inlineStyle('test.css')}</div>`); // <div>[object Promise]</div>

2. The source text of the CSS file is dropped in-place without any compilation or processing, meaning that any @import statements are sent to the browser, where they will fail unless specifically served via web_resources(). This prevents the header and footer components in the site example from using declarative shadow DOM.

I think we should update this API to be synchronous and leave an HTML annotation rather than reading form the file system at all. The bundling process could extract this annotation, read the file, generate a bundle, and inject it back into the HTML at the correct location. This would avoid the async problems and fix @import.

The challenge here is the implementation. Once the annotation is extracted, we need to leave a marker of some kind about where the CSS should be injected back in. Line and column numbers aren't stable since the file gets modified, and we need to be deterministic. I think we could just leave another marker which counts sequentially and then generate a file which maps each location index to the CSS file being inlined. We've have to generate an entry point for each and compile each one, but I think it should be doable.

[dgp1130]

Took a pass at this with my current progress in ref/inline-style-postcss. I was able to make inlineStyle() synchronous and extract a new annotation type, but when it comes to actually running postcss, I don't see a viable path.

The problem is that rules_prerender is designed to not require users to predeclare the files they are generating. This is really convenient for users as it allows them to easily generate large numbers of files, but it's a PITA for rules_prerender, since when bundling we know next to nothing about a user's build at loading or analysis time. Most tools (@bazel/rollup and rules_postcss in particular) expect inputs to be known at loading or analysis time. In this case, we need to run postcss for every CSS file that gets inlined onto a page. That happens when the user calls inlineStyle() in NodeJS code and is not known to Bazel. As a result, I can't get a list of a CSS source files to run postcss on early enough to actually pass it in to postcss_multi_binary().

I see three paths forward which are all pretty rough:

1. Improve postcss_multi_binary() to support execution-time entry points. I already filed an issue about this, but it didn't get much traction. We can update postcss_multi_binary() to accept a directory of sources (which is defined at execution-time) and process each of them as inputs, outputting each file to the same relative path in an output directory. I think this would be good enough for rules_prerender, but it's a pretty significant change in rules_postcss and likely would be difficult to land given the current implementation uses different actions for each source file.
2. Declare inlined styles in advance. We could require users to declare inlined styles up-front at loading time. For example:

   prerender_component(
       name = "my_component",
       styles = ["foo.css", "bar.css"],
       inlined_styles = ["foo.css"],
       # ...
   )

   This would all prerender_component() and prerender_pages_unbundled() to collect all the inlined styles into a single filegroup() and pass them to postcss_multi_binary().
3. Process all styles just in case they are inlined. Running a file through postcss itself doesn't accomplish anything, we have to inject the result back into the HTML page being built. The simplest solution is just process every CSS file in a compilation, just in case it ends up being inlined. We would then only inject styles that actually are inlined, so the end result is still correct.

Option 1. is probably the best as it makes the most logical sense, is the most performant, and doesn't pollute the user's code with extra boilerplate. Option 2. is awkward but not that bad IMHO, since a component should be simple enough to easily know what styles will be inlined. Option 3. is the most straightforward, but also the least performant since we'll be wasting a lot of time processing each and every CSS file only to actually use a small subset of them.

For testing right now I'll go with option 3 since that is the easiest to implement. Simultaneously, I'll explore option 1. and see if it is feasible and if there's appetite to land it on the rules_postcss side.

[dgp1130]

Asked in rules_postcss if this is something I could feasibly contribute (bazelbuild/rules_postcss#63 (comment)), I'll keep following up there to see if option 1 can make any progress.

[dgp1130]

Tried to make option 3 workable and came up with ref/inline-style-2. File resolution becomes quite complicated because the files are known at loading time, they must have different paths to the actual source files (or they would conflict). And since they are generated for each page (rather than each component), they must be in the same package as the postcss_multi_binary(), which is likely different from the source packages. This means we have to reroot the files like wksp/path/to/pkg1/postcss_multi_binary_target_name/path/to/pkg2/file.css. Doing so makes file resolution significantly more complicated, but it is doable at least. It may be possible to call postcss_multi_binary() at the prerender_component() level to keep consistent file paths, but that may prevent having the same CSS file be used by multiple prerender_component() targets (or one from a different package), so I don't think that's feasible but might be worth investigating.

We also run into an edge case where postcss_multi_binary() requires all srcs to emit at least one *.css file. That's a reasonable requirement, but we don't actually know if the user included any CSS files, and if they don't it fails the build. To work around this, I generated an empty CSS and always include it. I suppose users could accidentally import this empty file, but that seems unlikely.

While option 3 is viable, it overcomplicates file resolution, encounters more edge cases in postcss_multi_binary(), and still has the performance issue of processing all the CSS files when it almost certainly doesn't need to. I'll keep pushing on option 1 and see if there's any way I can make that happen.

[dgp1130]

Coming back to this a few months later, postprocessing inlined styles is the most immediate issue in rules_prerender which makes components difficult to write in an isolated fashion. I really want to be able to author some kind of theme.css or base.css which gets imported into every component which wants to use some common fonts, colors, or spacing. This requires two things:

1. A way to import "library" CSS files not declared by the component itself.
2. A way to declare and depend upon "library" CSS files.

For the first we have @import for clear syntax, the challenge is that we need to bundle styles that get inlined into a component. This runs into all the problems mentioned earlier.

For the second, we would need to define some kind of css_library() rule or use filegroup() which prerender_component() targets can depend upon.

Reflecting on the three possible solutions discussed earlier:

The first (update rules_postcss to support execution-time entry points) seems not viable as rules_postcss has been stalled for some time with little to no movement or attention from Google. This seems unlikely to change and I don't think it would be good to try to push this kind of change through. Even if I was successful, the next problem I run into is going to have the same issue. I field #46 to look into alternative CSS bundlers as a long-term solution to this problem.

The second (declare inlined styles on prerender_component()) is not the DX I really want to build here, however I see two possible improvements that weren't obvious to me before:

1. The style attributes doesn't need to be source files, it can be a reference to a css_library() which simplifies some of the dependency management concerns that would be placed on prerender_component() (ie. strict deps).
2. inlined_styles is awkward, but it is actually probably the more likely of the two to be used assuming declarative shadow DOM does succeed as the the best way to author a prerender_component() (which is still TBD for sure, but that's my hope at least). If so, inlining styles should really be the default option. The other one can be named global_styles to be clear that it applies to the whole page and needs to be manually namespaced. Arguably we shouldn't support global styles at all except on top-level page components, but that may be a bit idealistic.

The third approach (process all styles just in case they are inlined) is kind of a pain to implement with rules_postcss, though that might be improved by swapping out the CSS bundler. It's also very non-performant.

After feeling the pain of the third approach from prototyping and taking a another look at the second solution, I'm actually thinking number 2 might be the best path forward. I'll prototype that approach and see how well it works in practice. Global styles should be rare in reusable, isolated components, so most prerender_component() targets should just have the one styles attribute, and most of this complexity would be properly hidden.

I may need to rename includeStyle() to includeGlobalStyle() and inlineStyle() to includeStyle() for consistency and to incentivize local styles where possible. We'll also want to enforce that CSS files in the styles attribute are only ever used by includeStyle() while CSS files in the global_styles attribute are only ever used by includeGlobalStyle().

[dgp1130]

I prototyped the suggested modifications to option 2 in ref/inline-style-3 (yes I realize it's confusing that I prototyped each option in a different order than the options were numbered). I think this is actually a viable path forward. It has some interesting design decisions, but mostly seems to work.

I added an inline_styles attribute to prerender_component() which is a list of labels to css_library() targets. The css_library() rule doesn't really do anything, it just tracks direct and transitive dependencies. prerender_component() then creates a css_binary() for all css_library() targets, which resolves @import statements and bundles each direct source file. The renderer now does not directly inline CSS into the output HTML (which required a file read and an annoyingly viral await), but instead generates an annotation which later injects the bundled CSS file into that location as a <style /> tag.

This lead to a few interesting design decisions:

Firstly, prerender_component() is responsible for calling css_binary(), which is kind of weird since that's more of a bundling task. However I think it is necessary here so each inlined CSS library can be compiled independently with its own dependenices. If we did all inlined CSS files in a single postcss_multi_binary(), they would all share the same deps, which could hide strict deps issues.

Secondly, speaking of strict deps, this has none. css_library() doesn't do anything and while each library gets its own postcss_binary(), there's nothing prevent a CSS file from @import-ing a transitive dependency. We'd probably need to add a validation action to css_library() which detects and prevents this state. I'm not sure how hard this would be build, but I think it would be possible to generate a postcss_binary() for each library which accepts only direct sources and direct dependencies. I could include a custom plugin which validates that imports for direct sources resolve to something and ignore imports from dependencies (so we don't need to include transitive dependencies in the action). I'm not sure how involved this is, but I don't really feel like writing another PostCSS import plugin config in Bazel given how complex the last one turned out to be.

Thirdly, this will probably work with Sass, but I think it would require users to manually write sass_binary() targets and then wrap them in css_library() targets that feed into inline_styles. As long as sass_binary() resolves all the imports, and no @import statements are left in the output, css_library() and css_binary() should be a simple pass-through. In the future it might be worth seeing what we can do the streamline Sass support to at least cut out that css_library(), even if the rest of the build pipeline is unchanged.

Fourthly, I'm still not a fan of shipping my own version of css_library() at all. As written, I don't think css_binary() needs to be exposed to users (because it is called implicitly by prerender_component(), and I think that limits the difficulty of a future migration off this custom implementation if and when it becomes viable. Unfortunately there is still a lot of nuance here, particularly around how imports are resolved. Unfortunately there just is no better option right now aside from hard-requiring Sass perhaps.

Fifthly, I wrote everything with a new inline_styles attribute on prerender_component() just to minimize breaking changes where possible right now. I think once this feature is fully implemented, it should be named styles by default, with the current styles attribute being renamed to global_styles or something like that. Since "global styles" are just inlined in the <head /> tag, I'm hoping I can eventually drop global_styles altogether and just use styles at the prerender_pages() element to manually inlineStyle() in the <head /> tag. This should have feature parity with the current implementation, but might be a performance regression, since it means that global styles in different locations will exist in different <style /> tags and could be repeated if they include the same content. I'm hoping that if everything is done via local styles inlined in a declarative shadow root, there shouldn't be much need for global styles in components, but I'm not sure how realistic that is.

Sixthly, css_binary() targets generate a distinct CSS file from their input sources, as generally expected. The challenge with this is that users want to write imports for the source file. For example, if I have css_library(srcs = ["foo.css"]), I want to import it with inlineStyle('rules_prerender/.../foo.css');. However, the css_binary() target in prerender_component() would rename the file to something like foo_bin.css and break the import. I initially tried to keep the file name the same and generate with without a label via ctx.actions.declare_file() and returning it in the DefaultInfo() of the css_binary() rule. This worked, but I quickly realized that it doesn't support a css_binary() which compiles a css_library() from a different package. Even if the file name matches, the package won't. I ended up solving this by having css_binary() return a CssImportMap provider which maps import strings rules_prerender/.../foo.css (as they would be written in a prerender TS library) to the source file it should import. This allows the renderer binary to validate the strings given (at the binary level, it still doesn't support strict deps at the library level), and it means the inlineStyle() string can match the source file path and is decoupled from the generated binary CSS file name. It's a bit weird, but I think it actually works out quite well in practice.

The next step here is to try to clean up this prototype into a complete branch and avoid introducing regressions. I think I'll start by introducing the new inline_styles attribute, then rename styles to global_styles, then rename inline_styles to styles, and finally take a look to see if I can get away with deleting global_styles.

[dgp1130]

Production-quality work (not experimental) is ongoing in inline-styles. Currently I've got the CSS ruleset mostly defined and will continue to build from here.

[dgp1130]

I was able to land an initial implementation and just published it in 0.0.11. I've updated the README to demonstrate using inline styles and declarative shadow DOM since that's the preferred component model right now.

It's still called inline_styles and styles still exists as a separate implementation, but I was able to migrate all existing examples to use inline_styles without too much effort.

One interesting challenge I ran into was that using :%{component}_prerender_for_test doesn't depend on the renderer, so the inline style map isn't loaded. The easiest solution was to just allow the style map to not be set and leave the user's provided inline style alone. For tests like that, it really doesn't matter, though it kind of sucks that I can't provide a proper assertion there (see a720fff).

I think the next step is to rename inline_styles to styles and rename the existing styles to global_styles. That way it clearly prefers inline styles without users having to think about. We should also consider deleting global styles altogether since there's clearly not much use for it.

I'll close this for now since the feature is shipped. I opened #47 to follow up with renaming inline_styles to styles.