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.

Sign up for GitHub

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

Jump to bottom

Stop linking workspace root #49

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

Stop linking workspace root #49

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

Comments

@dgp1130
Copy link
Owner

dgp1130 commented Jul 4, 2022 •
edited
Loading

link_workspace_root = True is a bit of an antipattern. This appears to be going away in rules_js / rules_ts.

Instead, we 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.

Also see discussion in #8.
@dgp1130 dgp1130 added the cleanup Internal cleanup of infrastructure label Jul 4, 2022
@dgp1130 dgp1130 mentioned this issue Jul 4, 2022
Closed
@dgp1130
Copy link
Owner Author

dgp1130 commented Jul 4, 2022

As part of #50, I need to turn off link_workspace_root for a number of targets to avoid a conflict with package_name = "rules_prerender" in the declarative shadow DOM target. There's no easy path to fixing this conflict aside from dropping link_workspace_root everywhere.

Alternatively, we could rename the workspace to be a different name than rules_prerender, but given that link_workspace_root is going away in rules_js, I don't think this is a viable long term solution.

dgp1130 added a commit that referenced this issue Jul 4, 2022
@dgp1130
Migrates all example ts_library() targets to ts_project().
0cd2a67 
Mostly just works, but two major issues to overcome:

1. `ts_library()` generated ES6 sources which used ESM modules, but `ts_project()` just does what the `tsconfig.json` says, which is CommonJS by default. Need to use a separate `tsconfig` attribute for client-side scripts so they generate ESM JS which can be properly bundled by Rollup.
2. The `ts_project()` macro uses `link_workspace_root = True` by default which maps `rules_prerender` to `""` (the root package path). This is incompatible with `//packages/rules_prerender` which maps `rules_prerender` to `"packages/rules_prerender"` and cannot be used together. Test code which depends on `:%{component}_prerender_for_test` encounters this issue for reasons I don't remember. The easiest solution for now is to set `link_workspace_root = False` for such `ts_project()` targets and use relative imports. Long term we should move to a fully relative / package name import model. Filed #49 to follow up on that work.
dgp1130 added a commit that referenced this issue Jul 4, 2022
@dgp1130
Migrates all example ts_library() targets to ts_project().
c790059 
Mostly just works, but two major issues to overcome:

1. `ts_library()` generated ES6 sources which used ESM modules, but `ts_project()` just does what the `tsconfig.json` says, which is CommonJS by default. Need to use a separate `tsconfig` attribute for client-side scripts so they generate ESM JS which can be properly bundled by Rollup.
2. The `ts_project()` macro uses `link_workspace_root = True` by default which maps `rules_prerender` to `""` (the root package path). This is incompatible with `//packages/rules_prerender` which maps `rules_prerender` to `"packages/rules_prerender"` and cannot be used together. Test code which depends on `:%{component}_prerender_for_test` encounters this issue for reasons I don't remember. The easiest solution for now is to set `link_workspace_root = False` for such `ts_project()` targets and use relative imports. Long term we should move to a fully relative / package name import model. Filed #49 to follow up on that work.
dgp1130 added a commit that referenced this issue Jul 4, 2022
@dgp1130
Migrates all example ts_library() targets to ts_project().
3b16ddd 
Mostly just works, but two major issues to overcome:

1. `ts_library()` generated ES6 sources which used ESM modules, but `ts_project()` just does what the `tsconfig.json` says, which is CommonJS by default. Need to use a separate `tsconfig` attribute for client-side scripts so they generate ESM JS which can be properly bundled by Rollup.
2. The `ts_project()` macro uses `link_workspace_root = True` by default which maps `rules_prerender` to `""` (the root package path). This is incompatible with `//packages/rules_prerender` which maps `rules_prerender` to `"packages/rules_prerender"` and cannot be used together. Test code which depends on `:%{component}_prerender_for_test` encounters this issue for reasons I don't remember. The easiest solution for now is to set `link_workspace_root = False` for such `ts_project()` targets and use relative imports. Long term we should move to a fully relative / package name import model. Filed #49 to follow up on that work.
dgp1130 added a commit that referenced this issue Jul 4, 2022
@dgp1130
Migrates all example ts_library() targets to ts_project().
0bc4cc6 
Refs #50.

Mostly just works, but two major issues to overcome:

1. `ts_library()` generated ES6 sources which used ESM modules, but `ts_project()` just does what the `tsconfig.json` says, which is CommonJS by default. Need to use a separate `tsconfig` attribute for client-side scripts so they generate ESM JS which can be properly bundled by Rollup.
2. The `ts_project()` macro uses `link_workspace_root = True` by default which maps `rules_prerender` to `""` (the root package path). This is incompatible with `//packages/rules_prerender` which maps `rules_prerender` to `"packages/rules_prerender"` and cannot be used together. Test code which depends on `:%{component}_prerender_for_test` encounters this issue for reasons I don't remember. The easiest solution for now is to set `link_workspace_root = False` for such `ts_project()` targets and use relative imports. Long term we should move to a fully relative / package name import model. Filed #49 to follow up on that work.
dgp1130 added a commit that referenced this issue Jul 7, 2022
@dgp1130
Rewrites all absolute imports to use relative imports.
e15c939 
Refs #49.

Also removes the path mapping in `tsconfig.json` which was necessary to resolve these imports in editor.
dgp1130 added a commit that referenced this issue Jul 7, 2022
@dgp1130
Removes link_workspace_root = True for ts_project() targets.
1c3cd10 
Refs #49.

Since all imports are relative, this is no longer needed. Two usages still exist, both of which for client-side script loading, which is consumed by Rollup. That doesn't affect NodeJS execution, so it's a less signficant issue but something which will probably have to be addressed eventually.
@dgp1130 dgp1130 mentioned this issue Jul 7, 2022
Closed
@dgp1130
Copy link
Owner Author

dgp1130 commented Jul 7, 2022 •
edited
Loading

Successfully removed link_workspace_root = True by converting all absolute imports to relative ones. Not elegant, but it's the minimal solution to this problem.

There are still two link_workspace_root = True usages with rollup_bundle(), but that's a different problem I can tackle separately. I think this is good enough to call it fixed.

I'll follow up on the more general project restructuring in #51.

@dgp1130 dgp1130 closed this as completed Jul 7, 2022
dgp1130 added a commit that referenced this issue Jul 7, 2022
@dgp1130
Fixes renderer for external usages.
fffc7de 
Refs #49.

The previous commit broke the renderer for external use cases via NPM. The reason is because apparently the execution tree looked like:

```
.../renderer.runfiles/
  npm/
    rules_prerender/
      packages/renderer/renderer.js
      # And *no* other files.
    node_modules/rules_prerender/
      common/binary.js
      # And all the other files...
```

The entry point was `.../renderer.runfiles/npm/rules_prerender/packages/renderer/renderer.js`, however a relative import to `../../common/binary.js` failed because `renderer.js` was the *only* file in that source tree (only thing under `.../renderer.runfiles/npm/rules_prerender/...` (with the exception of declarative shadow DOM stuff which is unrelated). This previously worked because it imported `rules_prerender/common/binary.js` which got resolved to the `.../renderer.runfiles/npm/node_modules/rules_prerender/common/binary.js` file by coincidence.

The correct solution is to use `.../renderer.runfiles/npm/node_modules/rules_prerender/packages/renderer/renderer.js` as the entry point, so relative paths work as expected. This translates to the `@npm//:node_modules/rules_prerender/packages/renderer.js` target. Unfortunately this strategy is incompatible with `npm_install(exports_directories_only = True)`, but we haven't enabled that yet, so it's a problem to deal with later.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cleanup Internal cleanup of infrastructure
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@dgp1130