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

Restructure internal packages #51

Closed
dgp1130 opened this issue Jul 7, 2022 · 2 comments
Closed

Restructure internal packages #51

dgp1130 opened this issue Jul 7, 2022 · 2 comments
Labels
cleanup Internal cleanup of infrastructure

Comments

@dgp1130
Copy link
Owner

dgp1130 commented Jul 7, 2022

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 dgp1130 added the cleanup Internal cleanup of infrastructure label Jul 7, 2022
@dgp1130
Copy link
Owner Author

dgp1130 commented Jul 7, 2022

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
Copy link
Owner Author

dgp1130 commented Mar 17, 2023

#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

@dgp1130 dgp1130 closed this as completed Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Internal cleanup of infrastructure
Projects
None yet
Development

No branches or pull requests

1 participant