perf: don't set JsInfo declarations and transitive_declarations in npm_link_package_store #1554
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…m_link_package_store
gregmagolan
force-pushed
the
no_npm_link_package_store_declarations
branch
from
8e2370b
to
33afa7f
Compare
jbedard
approved these changes
Mar 29, 2024
dgp1130
added a commit
to dgp1130/rules_prerender
that referenced
this pull request
Jul 29, 2024
… removing workspace-local package dependencies. `types_only` breaks in `@aspect_rules_js` v1.40.1 because declarations are dropped from `npm_link_package_store` in aspect-build/rules_js#1554, therefore we need a different approach. Unfortunately there does not appear to be a clean way to depend on only the types of a module, so instead I added `prune_dependencies` which attempts to remove type-only dependencies after the fact. This allows `//packages/preact/...` to depend on the `rules_prerender` NPM package locally in the workspace, but then have that dependency removed. When the package is installed and linked in the user's workspace, `@user//:node_modules/@rules_prerender/preact` is linked against the user's `@user//:node_modules/rules_prerender` instead of `@rules_prerender//:node_modules/rules_prerender`. `//tools/binaries/renderer` usage of `types_only` is not strictly necessary as it (intentionally) does have a value import of `rules_prerender`. It does technically depend on the local copy of `@rules_prerender//:node_modules/rules_prerender`, but this is never executed at runtime so it's not a significant problem. As long as we limit it to type-only imports, it should be fine. `prune_dependencies` only works on an `npm_package`, meaning we can't use it the `:renderer` `ts_project`. I did consider adding another version of `prune_dependencies` which prunes `JsInfo` providers instead, however this seems to be a more complicated structure with file paths and nested providers which need to be pruned. Since it's not strictly necessary for `//tools/binaries/renderer`, I opted not to go down that route just yet, but it can be considered in the future. I also briefy attempted creating a `@rules_prerender/renderer` package just to apply `prune_dependencies` to that. However `:renderer` depends on content outside its directory such as `//common:formatters`, so this needs to be included in the package as well, and an `npm_package` in `//tools/binaries/renderer/...` cannot include content outside of it. I then tried moving the `npm_package` to `//:renderer_pkg`, but this requires a `package.json` in the same directory, which would conflict with the `rules_prerender` package which has the same constraint. So there's no easy way to hack around this. Instead, we'll just rely on avoiding a value import of `rules_prerender` in the renderer.
dgp1130
added a commit
to dgp1130/rules_prerender
that referenced
this pull request
Jul 29, 2024
This rule is broken by the next version of `@aspect_rules_js` in aspect-build/rules_js#1554, so we have dropped all usages of it. This is no longer needed.
|
Just FYI, I think there's a breaking change here which invalidates a I was able to fix things by depending on the regular NPM package (no limiting to only types) and then retroactively "pruning" the dependency by removing it from the |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on the rules_js 2.0 branch where I thought I needed to make changes to the JsInfo provider to get this perf improvement, I realized that the source of the issue is that
npm_link_package_storesetsdeclarationsandtransitive_declarationswhich it does not need to set since linked npm packages are already in runfiles for runtime dependencies and ts_project doesn't need them in declarations.The perf improvement here is that npm_package (when include_runfiles is set to False) will no longer get all npm packages in its manifest. In rules_js 2.0, npm_package include_runfiles can flip to False and this perf improvement will be the default case.
Type of change
Test plan