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

Publishable prerender_component() #39

Closed
dgp1130 opened this issue Jul 24, 2021 · 5 comments
Closed

Publishable prerender_component() #39

dgp1130 opened this issue Jul 24, 2021 · 5 comments
Labels
feature New feature or request

Comments

@dgp1130
Copy link
Owner

dgp1130 commented Jul 24, 2021

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 dgp1130 added the feature New feature or request label Jul 24, 2021
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
Copy link
Owner Author

dgp1130 commented Aug 16, 2021

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 bazel-contrib/rules_nodejs@5ad1596, but that is only in [email protected] 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 [email protected]. I tried making a minimal reproducing example of rules_postcss on [email protected], 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
Copy link
Owner Author

dgp1130 commented Aug 22, 2021

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 bazel-contrib/rules_nodejs#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 (bazel-contrib/rules_nodejs@2c2cc6e), 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 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
Copy link
Owner Author

dgp1130 commented Aug 28, 2021

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

[email protected] updates default NodeJS to v14, which includes a change that break @bazel/postcss. There is already a fix which landed bazelbuild/rules_postcss#69, but doesn't appear to have been released to NPM. Filed bazelbuild/rules_postcss#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 added a commit that referenced this issue Aug 28, 2021
…`4.0.0`, and NodeJS to `12.22.5`.

These are the latest Bazel and `rules_nodejs` versions, necessary to include some fixes/improvements for re-exporting `js_library()` and `ts_library()` targets, see #39 (comment).

`[email protected]` actually updates NodeJS to `14.17.5`, however this breaks `@bazel/[email protected]` and there is no later release for it. There is actually a fix, but it hasn't been released. I filed bazelbuild/rules_postcss#73 to request a release, but for now I'm keeping NodeJS at `12.22.5`, which is the current latest release that does not include the v14 breaking change. Note that NodeJS v13 is already unsupported as it is not an LTS release.

This is the most I can upgrade Bazel/`rules_nodejs` without breaking `@bazel/postcss`.
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 #39.

This allows common attributes like `visibility` and `testonly` to be used on the generated entry points.
dgp1130 added a commit that referenced this issue Aug 28, 2021
… a scripts dependency.

Refs #39.

This chooses to generate either a `ts_library()` or `js_library()` based on the sources provided to `prerender_component()`. Whatever is given, is re-exported via `js_reexport()` which propagates both `JSModuleInfo` and `JSEcmaScriptModuleInfo`. This needs to be custom because `js_library()` only returns `JSModuleInfo` while `ts_library()` returns both, but a `js_library()` that depends on a `ts_library()` will drop its dependency's `JSEcmaScriptModuleInfo`, meaning there is no clean way of re-exporting a target which may be a `js_library()` or a `ts_library()`.
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
…`4.0.0`, and NodeJS to `12.22.5`.

Refs #39.

These are the latest Bazel and `rules_nodejs` versions, necessary to include some fixes/improvements for re-exporting `js_library()` and `ts_library()` targets, see #39 (comment).

`[email protected]` actually updates NodeJS to `14.17.5`, however this breaks `@bazel/[email protected]` and there is no later release for it. There is actually a fix, but it hasn't been released. I filed bazelbuild/rules_postcss#73 to request a release, but for now I'm keeping NodeJS at `12.22.5`, which is the current latest release that does not include the v14 breaking change. Note that NodeJS v13 is already unsupported as it is not an LTS release.

This is the most I can upgrade Bazel/`rules_nodejs` without breaking `@bazel/postcss`.
dgp1130 added a commit that referenced this issue Aug 28, 2021
Refs #39.

This is a simple Starlark library for managing paths. For now it just has an `is_js_file()` function to return whether or not a file is JavaScript based on its extension. There are actually a few different possibilites so this is not quite trivial and I don't want to repeat this everywhere it is necessary.
dgp1130 added a commit that referenced this issue Aug 28, 2021
Refs #39.

This allows common attributes like `visibility` and `testonly` to be used on the generated entry points.
dgp1130 added a commit that referenced this issue Aug 28, 2021
… a scripts dependency.

Refs #39.

This chooses to generate either a `ts_library()` or `js_library()` based on the sources provided to `prerender_component()`. Whatever is given, is re-exported via `js_reexport()` which propagates both `JSModuleInfo` and `JSEcmaScriptModuleInfo`. This needs to be custom because `js_library()` only returns `JSModuleInfo` while `ts_library()` returns both, but a `js_library()` that depends on a `ts_library()` will drop its dependency's `JSEcmaScriptModuleInfo`, meaning there is no clean way of re-exporting a target which may be a `js_library()` or a `ts_library()`.
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 #39.

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

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
… a scripts dependency.

Refs #39.

This chooses to generate either a `ts_library()` or `js_library()` based on the sources provided to `prerender_component()`. Whatever is given, is re-exported via `js_reexport()` which propagates both `JSModuleInfo` and `JSEcmaScriptModuleInfo`. This needs to be custom because `js_library()` only returns `JSModuleInfo` while `ts_library()` returns both, but a `js_library()` that depends on a `ts_library()` will drop its dependency's `JSEcmaScriptModuleInfo`, meaning there is no clean way of re-exporting a target which may be a `js_library()` or a `ts_library()`.
dgp1130 added a commit that referenced this issue Aug 28, 2021
… a scripts dependency.

Refs #39.

This chooses to generate either a `ts_library()` or `js_library()` based on the sources provided to `prerender_component()`. Whatever is given, is re-exported via `js_reexport()` which propagates both `JSModuleInfo` and `JSEcmaScriptModuleInfo`. This needs to be custom because `js_library()` only returns `JSModuleInfo` while `ts_library()` returns both, but a `js_library()` that depends on a `ts_library()` will drop its dependency's `JSEcmaScriptModuleInfo`, meaning there is no clean way of re-exporting a target which may be a `js_library()` or a `ts_library()`.
dgp1130 added a commit that referenced this issue Aug 28, 2021
… a scripts dependency.

Refs #39.

This chooses to generate either a `ts_library()` or `js_library()` based on the sources provided to `prerender_component()`. Whatever is given, is re-exported via `js_reexport()` which propagates both `JSModuleInfo` and `JSEcmaScriptModuleInfo`. This needs to be custom because `js_library()` only returns `JSModuleInfo` while `ts_library()` returns both, but a `js_library()` that depends on a `ts_library()` will drop its dependency's `JSEcmaScriptModuleInfo`, meaning there is no clean way of re-exporting a target which may be a `js_library()` or a `ts_library()`.
dgp1130 added a commit that referenced this issue Aug 28, 2021
… a scripts dependency.

Refs #39.

This chooses to generate either a `ts_library()` or `js_library()` based on the sources provided to `prerender_component()`. Whatever is given is re-exported via `_js_reexport()` which propagates both `JSModuleInfo` and `JSEcmaScriptModuleInfo`. It sounds like we could use a `ts_library()` or `js_library()` re-export. However, we need to re-export *only* the `scripts` parameter but we must also depend upon component deps' scripts. So we need to re-export some targets as direct sources and other targets as transitive sources, which doesn't appear to be supported by either rule. As a reuslt, `_js_reexport()` is needed to fill this specific niche.

This required an awkward change to support Rollup. `rollup_bundle()` *requires* a label-reference to its entry point. It explicitly rejects a target that returns multiple files. We also need to use ES6 sources (`JSEcmaScriptModuleInfo` so ensure we don't accidentally use UMD sources in Rollup. Unfortunately, `JSEcmaScriptModuleInfo` actually provides two generated files for any one source file from `ts_library()` (a `foo.mjs` file and another `foo.externs.js` file). This means that Rollup *cannot* have an entry point dependency on a `ts_library()` target, because it will always generate two ES6 source files. Normally, you would get around this by having a direct reference to the `:foo.ts` file, which Rollup has special code to translate to the actual generated `foo.mjs` file. To work around this I had to update the generated `.ts` entrypoint to a `.js` entry point, and actually export the file itself beyond the `js_library()` it is included in. That way `rollup_bundle()` can have a label reference to the `.js` entry point without going through a `js_library()` or `ts_library()` indirection that would confuse it.
dgp1130 added a commit that referenced this issue Aug 28, 2021
Refs #39.

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

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. This also includes a dependency on another client-side `js_library()` which is defined with ESM and bundled via Rollup.
dgp1130 added a commit that referenced this issue Aug 29, 2021
…me bundle.

Refs #39.

This tests TypeScript depending on JavaScript for prerendering and client-side scripts. It also tests JavaScript depending on TypeScript for prerendering and client-side scripts. Most notably, for TypeScript to depend on JavaScript, the JS must define a `.d.ts` file, this is a strict Bazel requirement.
dgp1130 added a commit that referenced this issue Aug 29, 2021
…d a `js_library()` as a `scripts` dependency.

Refs #39.

This chooses to generate either a `ts_library()` or `js_library()` based on the sources provided to `prerender_component()`. Whatever is given is re-exported via `_js_reexport()` which propagates both `JSModuleInfo` and `JSEcmaScriptModuleInfo`. It sounds like we could use a `ts_library()` or `js_library()` re-export. However, we need to re-export *only* the `scripts` parameter but we must also depend upon component deps' scripts. So we need to re-export some targets as direct sources and other targets as transitive sources, which doesn't appear to be supported by either rule. As a reuslt, `_js_reexport()` is needed to fill this specific niche.

This required an awkward change to support Rollup. `rollup_bundle()` *requires* a label-reference to its entry point. It explicitly rejects a target that returns multiple files. We also need to use ES6 sources (`JSEcmaScriptModuleInfo` so ensure we don't accidentally use UMD sources in Rollup. Unfortunately, `JSEcmaScriptModuleInfo` actually provides two generated files for any one source file from `ts_library()` (a `foo.mjs` file and another `foo.externs.js` file). This means that Rollup *cannot* have an entry point dependency on a `ts_library()` target, because it will always generate two ES6 source files. Normally, you would get around this by having a direct reference to the `:foo.ts` file, which Rollup has special code to translate to the actual generated `foo.mjs` file. To work around this I had to update the generated `.ts` entrypoint to a `.js` entry point, and actually export the file itself beyond the `js_library()` it is included in. That way `rollup_bundle()` can have a label reference to the `.js` entry point without going through a `js_library()` or `ts_library()` indirection that would confuse it.
dgp1130 added a commit that referenced this issue Aug 29, 2021
Refs #39.

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. This also includes a dependency on another client-side `js_library()` which is defined with ESM and bundled via Rollup.
dgp1130 added a commit that referenced this issue Aug 29, 2021
…me bundle.

Refs #39.

This tests TypeScript depending on JavaScript for prerendering and client-side scripts. It also tests JavaScript depending on TypeScript for prerendering and client-side scripts. Most notably, for TypeScript to depend on JavaScript, the JS must define a `.d.ts` file, this is a strict Bazel requirement.
@dgp1130
Copy link
Owner Author

dgp1130 commented Aug 29, 2021

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 added a commit that referenced this issue Aug 29, 2021
Refs #39.

Left in this comment while debugging the NodeJS v14+ issue with `@bazel/postcss` as one possible solution (#39 (comment)). Ultimately decided to stay on Node v12, so it's no longer relevant and there is nothing to do.
dgp1130 added a commit that referenced this issue Sep 5, 2021
Refs #39.

This macro collects all the files generated by a `prerender_component()` target so they can be included in a published NPM package. This allows a component to be written in TypeScript and used normally within its source workspace, but then have its generated artifacts (`.js` and `.d.ts` in particular) published to an external package manager (typically NPM). Since we already support `prerender_component()` using `*.js` and `*.d.ts` sources, the published content can include its own, separate build file which makes its own `prerender_component()` that uses those generated artifacts. That component can then be depended upon by users of the package as if it were a regular `prerender_component()` compiled directly from source.

By doing this we don't ship TypeScript to NPM and we decouple the source files that actually generate a `prerender_component()` from the published files that end up on users machines. In particular, this means that we don't require a TypeScript compiler for the user, we don't need a `tsconfig` and we don't need to worry about compiler versioning for every build tool we use.

I debated a lot about whether or not we should include `*.d.ts` files for scripts, since they won't actually be type checked anywhere. However, I ultimately decided to include them because they can have useful information, there's not much harm in propagating them, and future or alternative bundling tools might take advantage of them. Understanding *why* certain `*.js` files don't have associated `*.d.ts` files would also be quite confusing and hard to trace back, so I think it's better to avoid that nuance to begin with.

It's tricky to automatically test this without a complex integration testing setup using multiple workspaces and passing through a `pkg_npm()` target. For now, we just assert that the files we expect are present in the output, which is good enough for the time being.
dgp1130 added a commit that referenced this issue Sep 5, 2021
Refs #39.

This mostly comes down to just including styles in the output `filegroup()`, but it also required some updates to the PostCSS import plugin configuration. The original implementation did not correctly handle loading CSS files from other workspaces because we previously assumed that the `execroot/` directory would contain all the workspaces for the action, but it actually only contains the primary workspace the action is running in. External workspaces are actually loaded under `execroot/${primaryWorkspace}/external/${externalWorkspaces}`. To add confusion, `npm` is one workspace, and everything installed via `node_repositories()` goes under there. So instead, I just updated the logic to look in directories directly under `external/npm/`, treating those as external workspaces. That's not entirely accurate, but usable enough for the DX I'm looking for.

Again, it's really hard to test the plugin configuration, but I've done so manually for now using [`ref/external`](https://github.com/dgp1130/rules_prerender/tree/ref/external/).
dgp1130 added a commit that referenced this issue Sep 5, 2021
…ource files.

Refs #39.

This is necessary to include transitive dependencies of the prerender library and the script libraries in the published package. It also includes transitive prerender and script declarations, which aren't strictly necessary but stil useful for debugging purposes and to avoid the inevitable question "Why do some `*.js` files not have associated `*.d.ts` files?".
dgp1130 added a commit that referenced this issue Sep 5, 2021
Refs #39.

This requires specifying the path mapping twice, once in the `BUILD.bazel` and again in the published `BUILD` file. This is kind of annoying but I don't have a better way of doing this atm.

It also leaks the implementation detail of the `%{name}_resources` target, since it gets included directly in the output package. I think this is ok since there is no better format it could be in (can't easily map a root-scoped `web_resources()` directory to a package-scoped component, multiple components might conflict, etc). This should be good enough for the time being at least.
dgp1130 added a commit that referenced this issue Sep 5, 2021
Refs #39.

External users can still benefit from this macro and it can encourage and promote developers sharing components more broadly and helping to develop the `rules_prerender` ecosystem.
dgp1130 added a commit that referenced this issue Sep 5, 2021
Refs #39.

This macro collects all the files generated by a `prerender_component()` target so they can be included in a published NPM package. This allows a component to be written in TypeScript and used normally within its source workspace, but then have its generated artifacts (`.js` and `.d.ts` in particular) published to an external package manager (typically NPM). Since we already support `prerender_component()` using `*.js` and `*.d.ts` sources, the published content can include its own, separate build file which makes its own `prerender_component()` that uses those generated artifacts. That component can then be depended upon by users of the package as if it were a regular `prerender_component()` compiled directly from source.

By doing this we don't ship TypeScript to NPM and we decouple the source files that actually generate a `prerender_component()` from the published files that end up on users machines. In particular, this means that we don't require a TypeScript compiler for the user, we don't need a `tsconfig` and we don't need to worry about compiler versioning for every build tool we use.

I debated a lot about whether or not we should include `*.d.ts` files for scripts, since they won't actually be type checked anywhere. However, I ultimately decided to include them because they can have useful information, there's not much harm in propagating them, and future or alternative bundling tools might take advantage of them. Understanding *why* certain `*.js` files don't have associated `*.d.ts` files would also be quite confusing and hard to trace back, so I think it's better to avoid that nuance to begin with.

It's tricky to automatically test this without a complex integration testing setup using multiple workspaces and passing through a `pkg_npm()` target. For now, we just assert that the files we expect are present in the output, which is good enough for the time being.
dgp1130 added a commit that referenced this issue Sep 5, 2021
Refs #39.

This mostly comes down to just including styles in the output `filegroup()`, but it also required some updates to the PostCSS import plugin configuration. The original implementation did not correctly handle loading CSS files from other workspaces because we previously assumed that the `execroot/` directory would contain all the workspaces for the action, but it actually only contains the primary workspace the action is running in. External workspaces are actually loaded under `execroot/${primaryWorkspace}/external/${externalWorkspaces}`. To add confusion, `npm` is one workspace, and everything installed via `node_repositories()` goes under there. So instead, I just updated the logic to look in directories directly under `external/npm/`, treating those as external workspaces. That's not entirely accurate, but usable enough for the DX I'm looking for.

Again, it's really hard to test the plugin configuration, but I've done so manually for now using [`ref/external`](https://github.com/dgp1130/rules_prerender/tree/ref/external/).
dgp1130 added a commit that referenced this issue Sep 5, 2021
…ource files.

Refs #39.

This is necessary to include transitive dependencies of the prerender library and the script libraries in the published package. It also includes transitive prerender and script declarations, which aren't strictly necessary but stil useful for debugging purposes and to avoid the inevitable question "Why do some `*.js` files not have associated `*.d.ts` files?".
dgp1130 added a commit that referenced this issue Sep 5, 2021
Refs #39.

This requires specifying the path mapping twice, once in the `BUILD.bazel` and again in the published `BUILD` file. This is kind of annoying but I don't have a better way of doing this atm.

It also leaks the implementation detail of the `%{name}_resources` target, since it gets included directly in the output package. I think this is ok since there is no better format it could be in (can't easily map a root-scoped `web_resources()` directory to a package-scoped component, multiple components might conflict, etc). This should be good enough for the time being at least.
dgp1130 added a commit that referenced this issue Sep 5, 2021
Refs #39.

External users can still benefit from this macro and it can encourage and promote developers sharing components more broadly and helping to develop the `rules_prerender` ecosystem.
@dgp1130
Copy link
Owner Author

dgp1130 commented Sep 5, 2021

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

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.

@dgp1130 dgp1130 closed this as completed Sep 5, 2021
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 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 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.
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