dgp1130 / rules_prerender

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

Restructure internal packages #51

Closed dgp1130 closed 1 year ago

dgp1130 commented 2 years ago

From #49, where we removed link_workspace_root = True:

[W]e should follow a model of:

  • Every JS file is associated with exactly one NPM package.
  • Some NPM packages may not be published, kept internal only.
  • JS files loaded by multiple NPM packages can use nested_packages to vendor common JS without having to publish internal packages.
  • Use relative imports inside a package.
  • Use absolute imports across packages.
  • Restructure the repository to define what these packages are and how fine/coarse-grained they should be.
    • Basically delete the common/ directory and split it into internal-only packages which are imported via their package names and vendored into each published package which uses them.

This likely means I'll also need to rethink the publishing story and possibly move to a @rules_prerender/* package model, something I want to do anyways, but haven't really gotten around to.

Currently everything is relative imported, but all across the repository which doesn't scale well and doesn't vendor into NPM packages.

dgp1130 commented 2 years ago

I tried playing around with nested_packages a bit but found it less than ergonomic. Much like how pkg_npm() normally works, you can't nest a package from a sibling directory, so having //packages/rules_prerender:pkg nest //packages/internal_dep:pkg has no effect. Instead, I still need the pkg_npm() target to live in the repository root, so having //:pkg nest //packages/internal_dep:pkg does work.

However, this nesting does less than I was hoping. I expected it to make the internal dependency available at the same package name, but it doesn't seem to do anything in that regard so you get errors like:

Cannot find module '@rules_prerender/annotations'. Please verify that the package.json has a valid "main" entry

The best solution I found was to add a substitution on the package to rewrite the internal package specifier (@rules_prerender/annotations in this case) with an "absolute" import internal to the package its called from (rules_prerender/packages/annotations). Since it's being imported from rules_prerender, the rules_prerender/... specifier always resolves to an "absolute" path under the package, and works when imported from any subdirectory in the package.

Unfortunately @aspect_rules_js's version (npm_package()) does not support nested packages or substitutions, so I think this approach won't be able to evolve with the new toolchain. I'll have to ask around to see if there are any suggestions as to the "right" way of doing this.

Current progress is in ref/nested-packages.

dgp1130 commented 1 year ago

67 has more concrete and actionable ideas here. Most of them have already landed to, so we can probably close this issue.

Duplicate of #67