search

dgp1130 / rules_prerender

A Bazel rule set for prerending HTML pages.
14 stars 0 forks source link

Publishable `prerender_component()` #39

Closed dgp1130 closed 3 years ago

dgp1130 commented 3 years ago

In #38, I wanted to create a prerender_component() in rules_prerender, which is published to NPM and loaded by user projects. This is tricky, because there is no way to consume a prerender_component() without compiling its TypeScript code and client-side scripts. This means that the only supported way of doing this is to ship the prerender and script TypeScript code to NPM and include a BUILD.bazel file which uses the prerender_component() target to compile it into something usable on the user's machine.

This is really awkward, requires TypeScript, and may not be compatible with the user's environment. If they are using a different TypeScript version or a different tsconfig, there is no guarantee that this compilation will succeed.

Ideally, we wouldn't ship TypeScript at all, and instead publish *.js and *.d.ts files, which are then consumed by a prerender_component()-like rule to downstream users. There is currently no supported way to use just JavaScript (see #4), so this is not possible today. Even if prerender_component() supported *.js and *.d.ts files, I'm not sure what module format or ES version we would want to use here. Since it just needs to be compatible with the user's bundling infrastructure which comes from the same rules_prerender package, it maybe doesn't matter so much. Of course, users could implement a custom bundler, but I think requiring ESM support there is a reasonable ask.

We should figure out a way to compile prerender_component() source files to *.js and *.d.ts, and then load and consume them from a user's project.

dgp1130 commented 3 years ago

I've been looking into this in order to unblock declarative shadow DOM #38, but found myself shaving quite the yak. Supporting JS is actually pretty straightforward and mostly comes down to using js_library() instead of a ts_library() in prerender_component(). The problem is making the rest of the toolchain work when given either a js_library() or a ts_library().

The biggest issue is that js_library() returns JSModuleInfo while ts_library() returns JSModuleInfo (with ES5 sources) and JSEcmaScriptModuleInfo (with ES6 sources). As a result, there doesn't seem to be a good way to "re-export" a target which could be either a js_library() or a ts_library(). Neither rule does this automatically and my attempts to write my own have failed miserably. Apparently rollup_bundle() only uses JSEcmaScriptModuleInfo if given (ignoring JSModuleInfo for that case). This appears to be fixed by https://github.com/bazelbuild/rules_nodejs/commit/5ad15960e69906a2c3e2484be9b17d1a77f83221, but that is only in rules_nodejs@4.0.0-rc.0 which hasn't hit stable yet. My attempts to use 4.0.0-rc.0 did actually build rollup_bundle() successfully, but breaks rules_postcss.

$ bazel run //examples/testonly:devserver                                                                                                                                                    (js|✚9)
INFO: Invocation ID: d47ebd15-f129-4ab2-9aa3-ccc01aa32ce1
ERROR: /home/dparker/Source/rules_prerender/examples/testonly/BUILD.bazel:10:16: in nodejs_binary rule //examples/testonly:page_styles.postcss_runner: 
Traceback (most recent call last):
        File "/home/dparker/.cache/bazel/_bazel_dparker/3b4ba8c4eec5951318c36cb07368e75b/external/build_bazel_rules_nodejs/internal/node/node.bzl", line 155, column 56, in _nodejs_binary_impl
                node_modules_manifest = write_node_modules_manifest(ctx, link_workspace_root = ctx.attr.link_workspace_root)
        File "/home/dparker/.cache/bazel/_bazel_dparker/3b4ba8c4eec5951318c36cb07368e75b/external/build_bazel_rules_nodejs/internal/linker/link_node_modules.bzl", line 72, column 47, in write_node_modules_manifest
                path = dep[ExternalNpmPackageInfo].path
Error: 'ExternalNpmPackageInfo' value has no field or method 'path'
Available attributes: direct_sources, sources, workspace
ERROR: Analysis of target '//examples/testonly:devserver' failed; build aborted: Analysis of target '//examples/testonly:page_styles.postcss_runner' failed
INFO: Elapsed time: 0.365s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 0 targets configured)
FAILED: Build did NOT complete successfully (0 packages loaded, 0 targets configured)

I'm not entirely sure what's going on here or if it is a bug in rules_postcss or rules_nodejs@4.0.0-rc.0. I tried making a minimal reproducing example of rules_postcss on rules_nodejs@4.0.0-rc.0, but this fails to load Skylib for reasons I can't explain:

$ bazel build //:styles
INFO: Invocation ID: a95f3b4c-9144-44fe-a665-927e10f9b0b9
ERROR: error loading package '': cannot load '@bazel_skylib//:workspace.bzl': no such file
INFO: Elapsed time: 0.186s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)

@bazel_skylib//:workspace.bzl definitely exists for version 1.0.3 which I'm using. But this error is so far removed from my real problem that I don't fully remember how it helps anyways.

My current work is in ref/tsjs, I'm not really sure how to move forward with this right now. I'll need to sleep on this and come back when I feel motivated again.

dgp1130 commented 3 years ago

After questioning my sanity for several hours, I eventually realized that the Skylib hash in new rules_nodejs projects is wrong. So Bazel was using a wrong/out of date (?) external repository for Skylib, which explains the missing @bazel_skylib//:workspace.bzl file. Made https://github.com/bazelbuild/rules_nodejs/pull/2880 to fix that, though I can also address this locally by just manually updating that hash.

I made a minimal reproduction of a postcss_binary() and did a binary search of rules_nodejs versions to find that 3.2.0 is the regressed version. That makes it seem like a bug rather than a breaking change, since this was a minor release.

Digging through commits in that version, this one looks suspect (https://github.com/bazelbuild/rules_nodejs/commit/2c2cc6eec42b0f4f805fb3063920107b78f821fd), as it is the one which adds path to ExternalNpmPackageInfo. Unfortunately this commit seems to add and require the path attribute, so it seems like it was forgotten somewhere else in the codebase.

Debugging further, I found that ExternalNpmPackageInfo is generated automatically by an aspect, however this appears to set path correctly. Adding some print statements, I eventually discovered that the broken target was the postcss_plugin() target, and it seems that it already provides an ExternalNpmPackageInfo, meaning that provider was used as-is, umodified by the aspect.

What confused me was that postcss_plugin() does not appear to actually return an ExternalNpmPackageInfo, however it does return an NpmPackageInfo. I then remembered that rules_nodejs has a lot of duplicate providers, and eventually realized that this is actually the same thing as ExternalNpmPackageInfo, just exported under a different name!

This postcss_plugin() target does not provide a path attribute, and since this error was introduced in 3.2.0, it should probably be fixed on the rules_nodejs side. I made a PR to default the path attribute to empty string, which seems to fix rules_postcss.

Unfortunately, I can't easily fix the issue locally to unblock work here since it's a regression in rules_nodesj which applies to rules_postcss, I'd have to patch one of the two to fix it. Hopefully the PR will land soon and make the 4.0.0 release, otherwise I can look into locally patching @bazel/postcss to set path = "", which should be enough to fix this particular issue until the fix lands in rules_nodejs.

dgp1130 commented 3 years ago

The fix for rules_nodejs landed in 4.0.0, so upgrading to that fixes the above error. Unfortunately, I then got more PostCSS errors:

(node:38) UnhandledPromiseRejectionWarning: TypeError [ERR_INVALID_ARG_TYPE] [ERR_INVALID_ARG_TYPE]: The "data" argument must be of type string or an instance of Buffer, TypedArray, or DataView. Received an instance of SourceMapGenerator
    at Object.writeFileSync (fs.js:1517:5)
    at /home/dparker/.cache/bazel/_bazel_dparker/3b4ba8c4eec5951318c36cb07368e75b/sandbox/linux-sandbox/1223/execroot/rules_prerender/bazel-out/host/bin/examples/styles/page_styles.postcss_runner.runner_src.js:213:16
(Use `node --trace-warnings ...` to show where the warning was created)
(node:38) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:38) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
ERROR: /home/dparker/Source/rules_prerender/examples/styles/BUILD.bazel:5:16: output 'examples/styles/page_styles_bundled.css.map' was not created
ERROR: /home/dparker/Source/rules_prerender/examples/styles/BUILD.bazel:5:16: Running PostCSS runner on <generated file examples/styles/page_page_styles.css> failed: not all outputs were created or valid

rules_nodejs@4.0.0 updates default NodeJS to v14, which includes a change that break @bazel/postcss. There is already a fix which landed https://github.com/bazelbuild/rules_postcss/pull/69, but doesn't appear to have been released to NPM. Filed https://github.com/bazelbuild/rules_postcss/issues/73 to ask for a release which should fix the problem.

The simplest workaround for now is to include:

node_repositories(node_version = "12.22.5")

Which will use the latest NodeJS version I can that doesn't contain the v14 breaking change (note that v13 is not LTS and has already fallen out of support).

dgp1130 commented 3 years ago

Just merged in most of the changes I had up to this point. You can now create a prerender_component() with JavaScript and it works roughly the way you would expect.

I still need to figure out how feasible it is to publish a prerender_component() and consume it from a different workspace. I believe it should be roughly as simple as publishing the *.js source files of the prerender_component() from the source workspace and then including another, published prerender_component() which accepts the *.js files and makes them available to the user's workspace.

dgp1130 commented 3 years ago

Using a JS prerender_component() in the published workspace actually works pretty well for this purpose. There were a few other problems I had to solve to make components truly publishable however.

Import path

The obvious import path for using published prerender code would be:

// /user.ts

import * as foo from 'rules_prerender/packages/some_component/foo';

This works, but needs to reach into private paths to do it. I spent a few hours trying to get package.json exports to work, but ultimately gave up. I'm not entirely sure where the problem was, but either the linker doesn't support it, or --bazel_patch_module_resolver is required somewhere, or the presence of exports makes Node read the sources as ESM. I'm not entirely sure which, or where the bug is / what I was doing wrong, but I ultimately gave up on it.

The solution I found was to use a TS re-export from the root of the published package.

// /some_component.ts

export * from 'rules_prerender/packages/some_component/foo';

This makes the component's prerender logic importable at rules_prerender/some_component as users would expect.

Loading scripts

Next problem was that Rollup can't find client-side scripts from another workspace. After a lot of trial and error I discovered two things need to be true:

  1. The client-side *.mjs files need to be in the bin directory to be found by Rollup. This means we need a copy_to_bin() target in the generated workspace.
  2. The client-side js_library() target needs to be in the workspace root package. This is so some provider fields on the js_library() are set correctly for the linker to load at the right path. This is a really awkward structure but fortunately not the end of the world since it can be managed in the publishing repository without users really noticing.

Styles

After that, loading CSS mostly worked, but I had a wrong understanding the PostCSS import plugin implementation as I expected all external workspaces to be alongside the primary workspace. That's actually not true as external (NPM) workspaces exist under external/npm/${package}. These technically aren't workspaces since only npm is a real workspace, but I had to adjust the resolve logic to look in the right directory.

See 7c8157821ce86e7c8c56ebb705db2f8bc063d70a.

Resources

Resources were the simplest and mostly "just worked", however they do leave an awkward my_component_name_resources/ directory in the published package. I think this is reasonable as it is just a consequence of the fact that the user can generate whatever they want in here and it doesn't make sense to expand that out into the real package.

Unfortunately most of these caveats apply to how publishable components are used rather than something that can be easily and centrally fixed. That means all the merged commits so far have almost none of this nuance. See ref/external for an example and that includes a published component.

I think for now we can call this issue closed. I think this is enough to return to the original motivation (declarative shadow DOM) and make a publishable component that does what I need.