refactor: rename npm_link_package_deps to npm_deps and npm_package_store_deps to npm_package_store_infos #1618
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
gregmagolan
force-pushed
the
rjs2_npm_deps
branch
2 times, most recently
from
f6652e9 to
f487110
Compare
…ore_deps to npm_package_store_infos
gregmagolan
force-pushed
the
rjs2_npm_deps
branch
from
f487110 to
16420b0
Compare
jbedard
reviewed
Apr 8, 2024
jbedard
reviewed
Apr 8, 2024
| @@ -4,8 +4,8 @@ JsInfo = provider( | |||
| doc = "Encapsulates information provided by rules in rules_js and derivative rule sets", | |||
| fields = { | |||
| "declarations": "A depset of declaration files produced by the target", | |||
| "npm_linked_packages": "A depset of files in npm linked package dependencies of the target and the target's transitive deps", | |||
| "npm_package_store_deps": "A depset of NpmPackageStoreInfo providers from non-dev npm dependencies of the target and the target's transitive dependencies to use as direct dependencies when linking downstream npm_package targets with npm_link_package", | |||
| "npm_deps": "A depset of files in npm dependencies of the target and the target's transitive deps", | |||
There was a problem hiding this comment.
Why add the term deps here while removing it elsewhere? None of the other fields here use the term after changing npm_package_store_* from "deps" to "infos"...
There was a problem hiding this comment.
It felt out of place on npm_package_store_deps. More reasoning on that here: #1620
npm_linked_packages doesn't seem like the right name either since the depset includes both linked package and transitive deps in the packages store.
Might be able to find something better than npm_deps. I'll open up another PR and we can bikeshed there.
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.
Improved naming for these so they are self-documenting.
Backward-compat handled in js_binary for the deprecated
include_npm_linked_packagesattribute.