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

Investigate alternative CSS bundlers #46

Closed
dgp1130 opened this issue Apr 15, 2022 · 10 comments
Closed

Investigate alternative CSS bundlers #46

dgp1130 opened this issue Apr 15, 2022 · 10 comments
Labels
feature New feature or request
Milestone

Comments

@dgp1130
Copy link
Owner

dgp1130 commented Apr 15, 2022

Currently we use rules_postcss for bundling CSS files, however it's current support level seems to be in a weird place (see bazelbuild/rules_postcss#73 and bazelbuild/rules_postcss#74). rules_prerender doesn't really have strict requirements here beyond resolving @import and the way rules_postcss does this is unnecessarily complicated IMHO. It would be worth investigating Parcel or ESBuild as an alternative CSS bundler which might be simpler, more modern, and more maintainable.

rules_nodejs has a Parcel example, though this seems intended for bundling JavaScript, not CSS.

Also see #34 for a separate investigation into ESBuild for bundling JS. Using the same tool for both pieces of the build could have other maintainability advantages.

@dgp1130 dgp1130 added the feature New feature or request label Apr 15, 2022
@dgp1130
Copy link
Owner Author

dgp1130 commented May 6, 2022

Tried using @parcel/css-cli via Bazel but quickly found that I couldn't actually execute it. It turns out that this package is implemented entirely natively and doesn't actually export any JavaScript, instead it uses a postinstall to download native binaries. npm_package_bin() doesn't work here and even trying to directly execute the binary via a genrule() doesn't work. Ultimately rules_nodejs sees the 'bin': { 'parcel-css': 'parcel_css' } in the package.json and generates a nodejs_binary() for it even though there's no JavaScript to execute.

I imagine this is still solvable as ESBuild works the same way but is directly supported in @bazel/esbuild. Taking a quick look they seem to retrieve this binary via a toolchain which gets pulled directly from NPM (not via npm install AFAICT).

The easier solution for now was to use @parcel/css which exposes a Node library for Parcel rather than a standalone binary. This still wraps the WebAssembly, but with a Node API which can be directly imported. I made my own nodejs_binary() which composed @parcel/css and it worked reasonably well. I was able to update css_binaries() to use Parcel and bundle its inputs to resolve and inline all imports.

The main challenge I haven't solved yes is that I had to use relative CSS imports for now for Parcel to resolve files correctly. Even that likely only works because the two files I was importing happen to both be source files. If one was generated, it would exist in a different artifact root and Parcel wouldn't know to look there. Workspace-relative imports also wouldn't work. Parcel does seem to support custom resolvers (example), so this could likely be solved. I'll have to experiment a little more.

Current progress is at ref/parcel.

@dgp1130
Copy link
Owner Author

dgp1130 commented May 7, 2022

I took a deeper look at a custom resolver, however it seems that @parcel/css doesn't actually support that. I'm unclear how .parcelrc custom resolvers work if not through this package, but it's possible their dependencies and interactions don't line up the way I think they do. I filed an issue to clarify my understanding and offered to contribute the feature if it is a reasonable ask.

@dgp1130
Copy link
Owner Author

dgp1130 commented May 8, 2022

In the @parcel/css issue I was directed at using the Parcel API from @parcel/core. I spent some time hacking on that and was able to eventually get something which worked.

Loading and calling the bundler was relatively straightforward. Unfortunately there doesn't seem to be a good way of defining an explicit output path, so I used an in-memory filesystem for Parcel and then manually forwarded the result to the real filesystem. I think it may be possible to get Parcel to do the right thing without this trick, but this should be good enough for now.

The biggest challenge was loading a resolver plugin. This turned out to be quite involved as plugins are defined in the .parcelrc configuration file and must be an NPM package of the name (@[^/]*)?/parcel-resolver-*. This becomes very difficult since I'm trying to use a locally defined plugin, not one which exists on NPM. Parcel does support local plugins, but they need to be loaded via NPM workspaces or Yarn link:, neither of which are easy to do within a Bazel context.

I eventually got Parcel to accept a provided .parcelrc file but it was struggling to import the resolver. I eventually got this working with npm_install(links = {"parcel-resolver-bazel": "//path/to/resolver"}) which makes an alias for @npm//parcel-resolver-bazel and sets up a node_modules/parcel-resolver-bazel directory with the contents of the given target. This was enough for Parcel to import the resolver. Unfortunately it is still quite limiting because it can't easily import other JS modules in the workspace (that would import from a node_module out to the main repository, which is a backwards dependency) and vendoring dependencies in the node_module directory becomes quite complicated. I was able to workaround this by sharing data with globals, but it is fairly limited and could be an issue in the future.

One other challenge I haven't fully solved yet is how to ship this plugin approach to downstream users. @npm//parcel-resolver-bazel won't exist in user workspaces because that's not a real NPM package. Some immediate ideas of how to solve this are:

  1. Document that users must add npm_install(links = {"parcel-resolver-bazel": "@npm//rules_prerender:parcel-resolver-bazel"}). This is awkward, easy to forget, and forces users to use NPM even if they don't want to do that (admittedly this is a requirement today for rules_prerender, but something I'm hoping to move away from eventually).
  2. Have rules_prerender workspace dependencies create our own npm_install(name = "npm_rules_prerender_internal") with an empty package.json and the single link for parcel-resolver-bazel, then always use that for the plugin. Not sure if this would cause conflicts with the @npm workspace when used in the same target.
  3. Actually publish parcel-resolver-bazel to NPM and depend on it in the rules_prerender NPM package or make users add the explicit dependency. This creates a pretty significant divergence between the way it works locally in rules_prerender and the way users actually consume it via NPM, but we already have some significant divergence here, so maybe that's just ok?

Depending on how hard getting this to work in external workspaces is, I'm starting to wonder if it would be easier to fork rules_postcss than to try and leverage Parcel in this manner.

I'll also follow up with Parcel folks to confirm that I really do have to go through all this NPM nonsense. It's a lot of ceremony to deal with in order to import a plugin, and there should definitely be a simpler way to do this.

@dgp1130
Copy link
Owner Author

dgp1130 commented May 8, 2022

Snapshot of the current state is in ref/parcel-2.

@dgp1130
Copy link
Owner Author

dgp1130 commented May 8, 2022

One other note: I had to abuse a global variable in order to have the CSS import map loaded into the Parcel binary get passed through the resolver plugin. Since Parcel manages loading and creating the plugin, there is no way to pass through use case specific context, and this was the best workaround I could find.

@dgp1130
Copy link
Owner Author

dgp1130 commented May 11, 2022

Latest suggestion is to implement the resolver in Rust since that API is actually supported. I hacked on it a bit and managed to get something working in ref/parcel-3, the Bazel side is infinitely simpler this way. That said I do have a few concerns:

  1. The parcel_css crate is still in alpha (version 1.0.0-alpha.24 as of today). I would rather not depend on prerelease code if I can avoid it, but also feel like 80% of the Rust crates I see haven't yet hit 1.0.0 despite being critical to the ecosystem.
  2. The resolver only see the full file path after Parcel has already joined it with the path of the originating file. That means that an absolute import of rules_prerender/foo.css from rules_prerender/path/to/bar.css results in a file resolution request for rules_prerender/path/to/rules_prerender/foo.css, which definitely doesn't exist. I think the problem here is that Parcel is assuming these specifiers work like typical file paths, when they technically don't in Bazel (${workspace_name}/... is treated as an absolute path). I could request a change in Parcel to get the originating file and the import specifier or accept that absolute imports aren't possible with typical Bazel syntax.
  3. Using the Rust toolchain means a lot more infrastructure I have to build out, as well as managing a new set of dependencies. In particular, I will need to figure out how to ship the precompiled Rust binary to users. That might involve compiling to WASM and calling it from JS, shipped to the rules_prerender NPM package, or I could explore bzlmod. Either way, there's a lot of open questions to figure out there.

This definitely feels better than trying to use the Parcel API, but there's a lot of new Rust-based problems I'm not familiar with which will take some solving before this becomes a viable approach.

@dgp1130 dgp1130 added this to the 1.0.0 milestone Jul 4, 2022
@dgp1130
Copy link
Owner Author

dgp1130 commented Jul 4, 2022

Since rules_postcss is locking this repository to Node v12 which is already unsupported, this is a hard blocker for a 1.0.0 release.

dgp1130 added a commit that referenced this issue Jul 17, 2022
Refs #46.

This is blocking the Node.js upgrade past v12 and the feature doesn't really work anyways as it isn't currently linked properly in the output. See: bazelbuild/rules_postcss#73.

Hopefully a future migration to Parcel will put us back in a position where CSS sourcemaps can be used.
dgp1130 added a commit that referenced this issue Jul 17, 2022
@dgp1130
Copy link
Owner Author

dgp1130 commented Jul 17, 2022

I just realized that the actual @rules_postcss issue only occurs when generating sourcemaps, however this never really worked in @rules_prerender anyways because it wasn't served correctly and I never bothered to get around to fixing it. Sourcemaps are a much smaller feature, particularly related to CSS files, so I think it's worth turning that off to unblock the Node.js upgrade. We can revisit CSS sourcemaps after to the move to Parcel. Done in 39a1fb6 and aafa060.

@dgp1130 dgp1130 removed this from the 1.0.0 milestone Jul 17, 2022
@dgp1130 dgp1130 added this to the 1.0.0 milestone Feb 14, 2023
@dgp1130
Copy link
Owner Author

dgp1130 commented Feb 17, 2023

In #48, we've now moved to @aspect_rules_js and should have an easier time supporting Parcel. This is the last major piece of infra which still requires @build_bazel_rules_nodejs.

I took a pass at this today and was able to make it work relatively easily. I'm not a fan of the dependency layout. parcel (the CLI) contains a direct dependency on @parcel/config-default (the default configuration). That might make some sense, except that the default config is basically an "everything config". It includes plugins for CSS, JS, GraphQL, Stylus, Yaml, etc. (source). This also means it depends on all those plugins. We only actually want it for CSS, nothing more. I made my own config which only included the CSS relevant plugins, however we still install all the plugins used by the default config, even though the default config isn't actually used.

After figuring out the dependencies I was able to make it work without too much effort. I got it working in an external workspace and was even able to remove all the PostCSS stuff, though Parcel is using PostCSS under the hood. I haven't bothered with strict deps yet, and it might be out of scope for this issue, but that would definitely be good to add sooner than later.

dgp1130 added a commit that referenced this issue Feb 18, 2023
Refs #46.

This tool calls `lightningcss` (formerly `@parcel/css`) to bundle the given entry points and output them to the specified locations.

I considered using the `parcel` CLI directly, but that included a lot of other functionality around compiling JavaScript, GraphQL, YAML, etc. `lightningcss` is a more narrowly defined package with a smaller dependency graph, which makes this easier to work with. It also includes a CLI, and I considered calling `lightningcss-cli` directly, but this was a bit easier to work with when bundling multiple files at the same time.
dgp1130 added a commit that referenced this issue Feb 18, 2023
Refs #46.

This adds a new `css_bundle()` target which calls the `css_bundler` binary (using `lightningcss`) to bundle all the given CSS files. This rule is then swapped in for `postcss_multi_binary()` for use across the whole project.

This necessitated updating `css_library()` to copy all its sources to the bin directory to be compatible with the `css_bundler` `js_binary()` target downstream. I would have liked to land this in a separate commit, but it actually breaks the `@bazel/postcss` pipeline since that relies on the bundled file having the same name as its entry point.

Partially for this reason, I ended up changing the output file paths of the bundled CSS files. Instead of generating the bundled CSS to use the same basename, I rerooted the whole execroot under the target. So if you bundle a file at `@some_wksp//path/to:file.css` at `@some_other_wksp//path/to/pkg:bundle` it will output the bundled version at:

```
.../execroot/some_other_wksp/path/to/pkg/bundle/some_wksp/path/to/file.css
```

Including the workspacename and putting everything under the bundle target means every input file as a 1:1 correlation with its output file, and there is no potential ambiguity or conflict.
dgp1130 added a commit that referenced this issue Feb 18, 2023
Refs #46.

The import map is only ever generated as part of `css_bundle()`. It needed to be a different rule before because `postcss_multi_binary()` was a macro and needed to be called at that level. But now that `css_bundle()` is a custom rule, we can generate the import map directly, which is much more straightforward.
dgp1130 added a commit that referenced this issue Feb 18, 2023
Refs #46.

These are no longer used since the implementation has been completely swapped over to Parcel.
@dgp1130
Copy link
Owner Author

dgp1130 commented Feb 18, 2023

Before I refactor and merge the branch using the parcel CLI directly, I do want to try using @parcel/css directly, since I think that might be the better level of abstraction to use for this problem. Apparently @parcel/css has been rebranded as lightningcss, but seems to be mostly the same thing.

After spending some time with that, I was able to make a css_bundler tool which calls lightningcss under the hood to bundle a number of entry points. This has significantly fewer dependencies and everything is actually appropriately used, so I think this is definitely the right path forward. Based on this I was able to create a new css_bundle() rule, simplify the CSS import map and delete all the PostCSS usage.

@dgp1130 dgp1130 closed this as completed Feb 18, 2023
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