-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
chore: refactor node module collector, extract explicit DependencyTree
, update types
#8872
Merged
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
…-node-module-collector-3
🦋 Changeset detectedLatest commit: 91165b1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
beyondkmp
reviewed
Feb 18, 2025
packages/app-builder-lib/src/node-module-collector/nodeModulesCollector.ts
Outdated
Show resolved
Hide resolved
packages/app-builder-lib/src/node-module-collector/nodeModulesCollector.ts
Outdated
Show resolved
Hide resolved
packages/app-builder-lib/src/node-module-collector/nodeModulesCollector.ts
Outdated
Show resolved
Hide resolved
beyondkmp
reviewed
Feb 18, 2025
packages/app-builder-lib/src/node-module-collector/nodeModulesCollector.ts
Show resolved
Hide resolved
packages/app-builder-lib/src/node-module-collector/npmNodeModulesCollector.ts
Outdated
Show resolved
Hide resolved
…-node-module-collector-3
0683e8e
to
1897be4
Compare
…ave different formats for `optionalDependencies`. make NodeModulesCollector into generic so that parse logic is consolidated to each subclass. add in optional deps just in case since we filter anything out that isn't in _dependencies in the subsequent reduce.
96b1120
to
f89179c
Compare
beyondkmp
reviewed
Feb 19, 2025
packages/app-builder-lib/src/node-module-collector/nodeModulesCollector.ts
Outdated
Show resolved
Hide resolved
beyondkmp
reviewed
Feb 19, 2025
packages/app-builder-lib/src/node-module-collector/nodeModulesCollector.ts
Show resolved
Hide resolved
beyondkmp
previously approved these changes
Feb 20, 2025
Great Job!!! |
This was referenced Feb 20, 2025
Merged
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.
types
for explicit usage of what is the resultJSON.parse
and what is used for the production treedelete array[key]
logic by refactoring toreduce
(no more modifying the originalJSON.parse
tree)JSON.parse
functionality to be specific to each collectorDependencyTree
with initial setimplicitDependenciesInjected = false
so that the boolean isn't optional by using DFS approachdependencies
NodeModulesCollector<T extends Dependency<T, OptionalsType>, OptionalsType>
JSON.parse
objects. Then uses a common method to construct the dependencies into aDependencyTree
and then internally (as before) hoisting and converting toNodeModuleInfo[]
optionalDependencies
withinpnpm list
(PnpmDependency
) andnpm list
(string
)