Update to @aspect_rules_js
#48
Comments
|
Migration doc - https://docs.aspect.build/aspect-build/rules_js/v1.0.0-rc.1/docs/migrate.html. Attempting to follow the instructions, switching to I'm a bit confused by the docs which indicate that "Link the node modules" comes before "Update WORKSPACE", feels like that order should be swapped? Also "Update WORKSPACE" is pretty lacking on details and how the existing workspace configuration should change. I tried dropping I also forced the Node version to stay the same at v12, though this technically isn't supported by Next problem is Also I still need Attempting to switch to which invalidates my hope of just rewriting imports. AFAICT there is no way to load Starlark code from an NPM package. That implies that I need to migrate all the rulesets I'm using to their Not sure where to go next. The docs suggest replacing any binary tool invocations from I tried updating
The error is definitely internal to I think I'm definitely lost on how these two rulesets are supposed to interoperate with each other. I'll reach out to some Aspect folks and see if I can get some direction here. I think some of the questions I have are:
Current process is in the |
|
Looking at some examples, it seems that you're not supposed to depend on Changing that dependency now builds the I get why that's an issue, resolving the path it would search for Refocusing on migrating Trying to migrate renderer struggled a bit with the entry point, since I needed to turn that into a relative import from the I feel like I eventually discovered this example which shows how to use This is the same problem removing The package should have The very hacky workaround to solve this is to move the I migrated all the examples to include
Both of these problems feel solvable, so I'll keep hacking on them. At this point I think I have a reasonable understanding of how
|
|
Fixed the styling issue by having the renderer still generate HTML with execroot-relative paths to CSS files. This is doable because it takes an inline style map as an input from Starlark, which maps "style import specifier" -> "execroot-relative path to CSS file". So even though the renderer is working in a single source tree with all root-relative paths, this map can still use an execroot-relative path and stay compatible with resource injector. One other challenge I encountered in this process is that the inline style map actually wasn't working. I found that the map would be set correctly in the renderer, but then when it was read it would always be undefined. It seems that the issue here comes back to package management. Since The solution here is to only use one version of the file. I updated the Moving on to declarative shadow DOM, merging with the Therefore, I think the better solution is to put declarative shadow DOM into its own NPM package. To do that I need to take its published files and dump them into an I think we would need to define an NPM package format for shipping {
"name": "@rules_prerender/declarative_shadow_dom",
"rules_prerender": {
"components": {
"declarative_shadow_dom": {
"prerender": ["./index.js", "./index.d.ts"],
"client": ["./polyfill.js", "./polyfill.d.ts"],
"styles": ["./styles.css"],
"resources": {"/declarative_shadow_dom/logo.png": "./dsd_logo.png"}
},
}
}
}Then you'd need to extract this component from the npm_link_all_packages(name = "node_modules")
published_prerender_component(
name = "rules_prerender_components/@rules_prerender/declarative_shadow_dom",
package = ":node_modules/@rules_prerender/declarative_shadow_dom",
component = "declarative_shadow_dom", # The component extracted from the `package.json`, because it could contain multiple.
)
prerender_pages(
name = "site",
# ...
# Use like `//:node_modules/*`, but where a `prerender_component` is needed.
deps = [":prerender_components/@rules_prerender/declarative_shadow_dom"],
)The challenge here is that the implementation of Since the core challenge seems to be that If we accept that users will depend on the A middle ground might be to use a single NPM package with two entry points like: import { prerenderMyComponent } from 'some_component/prerender';
// vs.
import { doSomethingOnTheClinet } from 'some_component/client';This would still avoid importing across contexts, but you would be depending across contexts, which isn't great either. I asked on the Bazel Slack and confirmed that publishing Starlark via NPM is no longer supported. So I think exporting a real Not sure what to do with that. |
|
Talked today with @alexeagle and @gregmagolan about some of the challenges (thank you both!). To write down my takeaways while they're still fresh in my mind:
First, we split JS and Starlark code. Ship only userland Answer: It doesn't. Splitting the JS and Starlark does mean there is a potential for version skew. I'm thinking A similar trick can be done for published There are still a few open questions on the
I feel confident that I can get the |
|
Tried to make something work here. I started by shifting all usages of the const { context } = require('rules_prerender');
const { execute } = require('path/to/packages/renderer/renderer.js');
execute(context);
Despite this, I was still seeing I knew I would want to drop these JS sources, I was just being lazy about it and didn't expect that to be required. I defined a new The next problem is that the renderer test code cannot import This code is generated by the test with a The test itself does have a dependency on Running Starting on the declarative shadow DOM problem, I made a new {
"components": {
"declarative_shadow_dom": {
"prerender": [
"declarative_shadow_dom.js",
"declarative_shadow_dom.d.ts",
"declarative_shadow_dom.js.map"
],
"scripts": [
"declarative_shadow_dom_polyfill.js",
"declarative_shadow_dom_polyfill.d.ts",
"declarative_shadow_dom_polyfill.js.map"
],
"styles": [],
"resources": "declarative_shadow_dom_resources/",
}
}
}Then, I've got a TypeScript can't process the import to This is enough for the renderer to successfully invoke the declarative shadow DOM component and render the HTML output 🎉. Unfortunately Rollup fails to bundle as it doesn't seem to be getting the The I could possibly downgrade Rollup, but apparently A side effect of switching to With Node.js updated to version 16.10.0 and using |
|
I tried migrating some of the ts_project(
name = "proj",
srcs = ["source.ts"],
tsconfig = {}, # Removes the requirement for a package-local `tsconfig.json`.
extends = "//:tsconfig", # Must be `@aspect_rules_ts` version of `ts_config()`.
declaration = True,
source_map = True,
deps = [":dep"],
)This mostly works the way I want it to. It doesn't require a package-local
I tried setting up I was also able to move the Jest change before the |
|
I tried running Jasmine within I did run into one novel issue where I also discovered that I got all the Looking in the test runfiles, I don't see Looking in I'm unclear how Node is supposed to find those packages though. Interesting, Note that This seems to mostly mirror the pnpm package structure: https://pnpm.io/symlinked-node-modules-structure. Therefore I figured this might be a pnpm-compatibility bug in The equivalent does not exist in Trying to understand why this directory exists, I eventually found this discussion which mentions the The migration guide mentions
|
|
I made a fix for WebDriverIO in webdriverio/webdriverio#8542, hopefully it'll land soon. In the mean time, I discovered npm_translate_lock(
# ...
# Links `debug` to the "" (root) Bazel package, so it becomes available at `//:node_modules/debug`.
public_hoist_packages = {
"debug@4.3.1": [""],
},
)Then I can add |
This makes it available even to packages which don't declare a dependency on it, which is necessary to work around a pnpm-compatibility issue in WebDriverIO. See: #48 (comment)
Refs #48. Generated with `pnpm import`. Also needed to disable strict peer deps as `@bazel/concatjs` requires Karma peer deps even though those rules are not used in `rules_prerender`.
Refs #48. This does not replace the `@npm` workspace, but instead adds a new `@npm_rules_js` so the two can co-exist.
Refs #48. This is an issue for the `@aspect_rules_js` version of `jasmine_node_test()` and wasn't load-bearing for `@build_bazel_rules_nodejs` anyways, so the simplest solution is to just delete it. Generated this commit with a simple find/replace in VSCode, looking for `import 'jasmine';\n\n` and replacing with "". Then did another search for `import 'jasmine';` and fixed a few stragglers which fell through the cracks.
Refs #48. Version `4.0.4` has a bad `package.json` which declares the `license` key twice and breaks `@aspect_rules_js`. This upgrades it to a later version with a fixed `package.json`.
Refs #48. This makes it available even to packages which don't declare a dependency on it, which is necessary to work around a pnpm-compatibility issue in WebDriverIO. See: #48 (comment)
Refs #48. This exposes `//:node_modules/rules_prerender` which can be used like any other NPM package yet is actually built from `//packages/rules_prerender` at HEAD. This _should_ be defined in `//packages/rules_prerender/...` but can't because it depends on files outside that Bazel package like `//common/...` which can't be pulled into the NPM package. Easiest solution is to move the `npm_package()` target to the root BUILD file, so _everything_ is a subpackage. However this does result in a weird file layout in the NPM package and clutters the BUILD file. Once the repository is reorganized to use internal NPM packages for common code, this can likely be moved to `//packages/rules_prerender/...`. Left a TODO to follow up with that.
Refs #48. This commit includes a handful of changes which needed to be landed all at once to avoid an awkward migration period. First we need to actually change the `jasmine_node_test()` macro to use `@aspect_rules_js`. There is no `@aspect_rules_jasmine`, so this needs to be done by manually invoking the `jasmine` binary via the rule generated by `npm_translate_lock()`. It generates a config file which explicitly lists its direct dependencies as test files. This means there is no convention to follow for test files (like `_test.js` or `.spec.js`), the only requirement is that it is a direct dependency of the `jasmine_node_test()` target. The config file needs to be passed in `args`, however that only works for direct `bazel test` invocations. The `web_test_suite()` target wraps this binary and drops `args`, so they need to be repeated in that context. This breaks abstraction a little bit, but is small and simple enough to not be too big a deal. Second we need to fix the errors this casues: 1. Adds `//:node_modules/*` dependencies where needed. `@npm//*` deps are still needed to type check things, but won't resolve at runtime. These dependencies are duplicated for now which doesn't seem to cause problems. Eventually the `@npm//*` dependencies will be dropped altogether. 2. Removes `@bazel/runfiles` from `jasmine_node_test()` targets because it fails (throws "`$BAZEL_WORKSPACE` is not set" error) and because it isn't needed. Since everything in `@aspect_rules_js` is in a single file tree, runfiles are just available at their root relative paths. This removes all but one usage of `@bazel/runfiles` which is still needed for a `nodejs_binary()` target. 3. Adds `debug` dependency to `//common/testing:webdriver`. This is necessary so `devtools` (a transitive dep of `webdriverio`) can import `debug` without an explicit dependency on it. This is a workaround for a pnpm-compatibility bug in WebDriverIO which should be removed once [the fix](webdriverio/webdriverio#8542) lands.
|
After that the One challenge is that Solution is to move the I also found that With that, I chunked off the Jasmine migration into its own branch, cleaned up the commits, and merged them. I figure that's a safe start to the migration since it's internal changes to testing which don't actually affect the built artifacts. |
|
CI failed after merging because apparently |
|
webdriverio/webdriverio#8542 landed but isn't released yet. Should be in v7.20.8 once that is ready. aspect-build/rules_js#301 also landed but isn't out yet either. Should be in v1.0.0-rc.3. |
… for `--config incompatible` builds. Refs #48. Since `@aspect_rules_js` has been updated to include aspect-build/rules_js#301, we can now re-enable this flag.
Refs #48. This is no longer used as it has been replaced with a custom `@aspect_rules_js` implementation.
Refs #48. This pulls in webdriverio/webdriverio#8542, which is needed to support pnpm natively.
Refs #48. Now that webdriverio/webdriverio#8542 has been pulled in, this package no longer needs to be hoisted.
|
The upstream Moving forward, I think the next incremental step I can do is take a tool and ship it via Experimenting with this, I updated the
I believe the reason for this is because I think the root cause here is that Updating the user
I think this is the same issue but applied to the I updated the user's After working through those issues, I found that In theory, I think everything we need to compile a component should be under Printing out more information, I see that this is actually the renderer which is failing to build because of a dependency on Rewriting that to Next error is that my That's enough to pass Building with Apparently generated inputs are under This is enough to build and extract annotations, however client side bundling fails with the error:
I'm a bit surprised by this since I would expect Rollup bundling to be unaffected by this change. It has a dependency on However, I think it's expected that the sources are under Digging through I tried looking through And with that an external application finally works with an The other question is: Do I really care about hard-coding I'm inclined to just hard-code the workspace names for now (maybe Current snapshot: ref/rules-js-annotation-extractor-external. |
|
@dgp1130 Heads up that I just landed a big refactor in rules_js that doesn't affect the user facing public API very much but it does affect the API for derivative rules. It adds a common provider (JsInfo) that downstream rules should use, establishes patterns to use and provides helpers for these. The change will go out in rc4 shortly. You can look at our rulesets for examples of how it affects downstream rules, aspect-build/rules_esbuild#48 LMK if you have a questions or ping me on Bazel slack if you would like some help with the changes required. |
|
Thanks @gregmagolan, I'll take a look once that comes out. Is the core change that there's a (new?) JsInfo provider which needs to be manually propagated in some situations and used where DefaultInfo suffices today? |
Refs #48. This mostly works just fine. I did notice that source maps seem to be broken, though that seems like a problem with the new `@aspect_rules_js` toolchain rather than this specific external example. I left a TODO to follow up with that separately.
|
I added an actual script to the example and found it bundled successfully, though I noticed two interesting things. First, import 'script';And following Node module resolution, that would be treated as an NPM package, not a relative path. So I can understand why Rollup doesn't resolve that, though I'm surprised that it was an issue with other paths. Eventually I realized the script entry generator adds Second, source maps don't seem to be working anymore. Even in non-external examples sourcemaps don't resolve correctly. It looks like TypeScript and Rollup aren't composing each other correctly given that the JS source files also seem to contain a source map reference at the end. They also reference files like Otherwise I was able to get client side scripts bundling and executing as expected. Next step is styles and then declarative shadow DOM to tie them all together. |
Refs #48. This mostly works but requires `postcss-import` to be installed in the user's workspace. It doesn't bother me that much right now though, since this is all still on `@build_bazel_rules_nodejs` infra. Once we update to Parcel with `@aspect_rules_js`, this shouldn't be an issue.
Refs #48. This just worked without any issues. Technically `prerender_pages()` emits a `WebResources` provider and follows the same code path, so this really has been working for a while now.
Refs #48. I actually want to bump the `@aspect_rules_ts` dependency, but this appears to require `@aspect_rules_js` to be bumped as well, and there seems to be an unintentional breaking change when used with `@aspect_rules_jasmine`, so we need to upgrade this first. See notes in https://github.com/aspect-build/rules_js/releases/tag/v1.17.0.
Refs #48. I actually want to bump the `@aspect_rules_ts` dependency, but this appears to require `@aspect_rules_js` to be bumped as well, and there seems to be an unintentional breaking change when used with `@aspect_rules_rollup`, so we need to upgrade this first. See notes in https://github.com/aspect-build/rules_js/releases/tag/v1.17.0.
Refs #48. I actually want to bump the `@aspect_rules_ts` dependency, but this appears to require `@aspect_rules_js` to be bumped as well.
Refs #48. This pulls in aspect-build/rules_ts#310 to fix `ts_project()` builds in external workspaces, meaning we can remove the local patch.
Refs #48. To use published prerender components from an external workspace (such as linking against the `npm_package()` for `@rules_prerender/declarative_shadow_dom`), that component needs to build within an external workspace. This has unique implications on the file paths since everything goes under `external/my_external_wksp/...`, but this is not reflected in `file.short_path` in a useful way. Instead we need to do a little more work to get the right paths, and `@aspect_bazel_lib's` `to_output_relative_path()` is exactly what we need. Essentially we should always prefer that over direct usage of `file.short_path`.
Refs #48. This actually worked a lot easier than I expected. `link_prerender_component()` worked just fine and the only real hiccup was adding `include_external_repositories = ["rules_prerender"]` to work around bazel-contrib/bazel-lib#359.
Refs #48. Need to refactor the way that `tsconfig.json` was managed and also realized that I needed to have a separate `tsconfig.client.json` to make sure it was compiled targetting ESM.
Refs #48. This the `rules_prerender` and `@rules_prerender/declarative_shadow_dom` `package.json` files which the current version. I initially tried to use [`stamped_package_json()`](https://docs.aspect.build/rules/aspect_rules_js/docs/npm_package#stamped_package_json), but found it insufficient because `@rules_prerender/declarative_shadow_dom` includes a peer dep on `rules_prerender` which was not being stamped. I filed aspect-build/rules_js#866 to suggest this as a general feature, but opted to implement my own version for now. It also uses `jq()` under the hood to transform the input JSON. It stamps the `version` property and any `rules_prerender` or `@rules_prerender/*` dependencies. I also add a `default_version` attribute. Not so much because it is actually useful, but because it makes the rule easier to test. Technically these tests fail with `--stamp`, but I don't think that should be too much of an issue.
|
Started looking into CSS in the external example and was able to get it working relatively easily. Needed to add I also added an image resource to the external site which worked as expected without any issues. Technically While I was working on that, aspect-build/rules_ts#310 landed with the fix for building The final problem is getting declarative shadow DOM working. I started by adding a Immediate error is that merging resources fails due to a bad file path: Ultimately this is because After landing that fix, I was able to build I think this is actually in a good enough state to release right now. There's definitely a lot of cleanup and refactoring which can be done, but it's good enough to be usable, even if set up is significantly more complicated than it should be right now. I do need to figure out the release process, given that it's a bit more complicated now. Previously I would just release the The first problem there is stamping the packages, which I've ignored so far. I tried to use Next I'll need to reevaluate the release action to publish both NPM packages simultaneously. Ideally it would also create the GitHub release and include the instructions for how to set up the |
Refs #48. This generates a binary which publishes the given package to NPM. It just merges the given directory with the root `.npmrc` file (which contains authentication information) and calls `npm publish`. Also made publish binaries for `rules_prerender` and `@rules_prerender/declarative_shadow_dom`. I tried to add a trap which deletes the staging directory after the binary has run, but I got permission errors and couldn't easily figure out what was going on. Fortunately `mktemp` still creates files under `$TMP`, so they should be cleaned up automatically, just a little less agressively than they could be.
|
Unfortunately it doesn't seem that publishing NPM packages is directly supported by In theory I don't think it should be that hard, given that we already have an NPM binary and the GitHub action includes authentication. I think all we need is an I developed my own For now, I generated the release notes by hand, this should be integrated with the release process. I found I think this issue has gone on long enough, and we've officially reached the point where I was able to successfully release against |
Refs #48. I previously updated the styles to reference `span { /* ... */ }`, but forgot to update the HTML.
Refs #48. Declarative shadow DOM now works and this test passes.
…a custom rule. Refs #48. Now that `//tools/internal:script_entry_generator` is compatible with `exec` configuration, it can be used in a `js_run_binary()` which simplifes the implementation a bit.
Refs #48. The renderer should not depend on `rules_prerender` directly, because that would pull in the JavaScript sources into `.../execroot/rules_prerender/node_modules/rules_prerender/` of the generated binary. The `rules_prerender` dependency should actually be linked against the user's implementation in their own workspace (`//:node_modules/rules_prerender`), not `@rules_prerender//:node_modules/rules_prerender`. However, we still want type checking of `rules_prerender` usage, even if the actual package gets injected via the main function parameter. `types_only()` accomplishes this by dropping all the JS sources and only propagating TypeScript declarations. This prevents the `rules_prerender` package from `@rules_prerender//:node_modules/rules_prerender` leaking into the renderer binary, so there is only one definition of it which comes from the user's workspace. `import type` also helps restrict that the dependency should _only_ ever be used as type. Unfortunately, `npm_link_package()` emits a `declarations` field which points to the NPM package's `TreeArtifact`, and that tree contains `*.js` files. That means TypeScript actually _does_ receive `*.js` files and is generally ok with value imports of `rules_prerender`. There's also no easy way of removing `*.js` files from that tree. This is an unfortunate foot-gun, but the most important thing is that the `*.js` files don't persist at runtime, even if the compiler may allow it. I filed a feature request to `@aspect_rules_ts` with this general idea and also explains more the `npm_link_package()` foot-gun: aspect-build/rules_ts#319.
Refs #48. This deletes or updates `TODOs` where follow up issues have been filed separate from the `@aspect_rules_js` migration. I generally deleted most of the `TODOs` because I personally feel that this kind of information should be tracked in issues. However I left a couple as notes of exactly where to cleanup when certain tasks are complete.
|
Congrats on landing, sorry I never had time to read all the text :) |
The future of the Bazel Node ecosystem appears to be
rules_js. It is still in RC right now but hopefully coming to stable soon. Would be worth playing around to see if it can improve things. The model of running actions inside a single source tree could smooth over some rough edges withts_project()and tools which insist on thenode_modules/structure, like Parcel.The text was updated successfully, but these errors were encountered: