-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feat(core): add glob support for project-level implicit dependencies #13138
feat(core): add glob support for project-level implicit dependencies #13138
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
06f51e4
to
e49243d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should mention this in the docs
8499aee
to
b9ed938
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's talk offline about this.
b9ed938
to
65ac73c
Compare
65ac73c
to
d0123c1
Compare
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 8ec97e3. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 11 targets
Sent with 💌 from NxCloud. |
d0123c1
to
2eaf94c
Compare
2eaf94c
to
d0d429b
Compare
d0d429b
to
7f3ebcc
Compare
7f3ebcc
to
c49dd21
Compare
c49dd21
to
b0e4b57
Compare
b0e4b57
to
d16654d
Compare
d16654d
to
ebd81fb
Compare
@@ -0,0 +1,35 @@ | |||
import minimatch = require('minimatch'); | |||
|
|||
export function findMatchingProjects( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you extracted this into a separate utility which is used in more than place, you can add some tests for this utility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thats a good shout out. I'll get them added.
implicitDependencies: ProjectConfiguration['implicitDependencies'], | ||
context: ProjectGraphProcessorContext | ||
) { | ||
return findMatchingProjects( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should rewrite this function. Currently the normalization of implicit dependencies is O(N^2), where N is the number of projects. In the worst case scenario it will be O(N^2) if say every project depends on every other project via '*', but his worst case scenario is unlikely.
What I would do is:
- Check if implicitDependencies is empty and skip the whole thing altogether.
- Check if there are some implicitDependencies using say '*', and if not, just assign them as project names.
- And only if there are implicitDeps that have special characters, I'd filter our the source and do the matching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can add some of the short-circuiting logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pushing up the changes here now. To summarize, this is the approach I took:
- Updated
findMatchingProjects
to short circuit if the nameOrPattern doesn't contain special characters. In this case, it will add it if its a valid project, and skip the glob matching entirely regardless. - If implicit dependencies is null or an empty array, we don't call into findMatchingProjects at all and simply return an empty array.
- We pass in the list of projectNames to
normalizeImplicitDependencies
. This should be the biggest boost in perf compared to the last pass at this. Now we find any matching projects, and then filter the source project out of the matches. This should mean filtering a much smaller array for large workspaces.
9d08b22
to
2fa8b08
Compare
2fa8b08
to
d512b67
Compare
d512b67
to
8ec97e3
Compare
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
Current Behavior
There is no way to specify all other projects as an implicit dependency of project {x}.
Expected Behavior
You can specify "*" in implicit dependencies to specify all other projects as a dependency. This is especially useful in the case of the root project, which may depend on all other projects.
Additionally, if you wanted to specify all other projects, except {x} as a dependency of {y}, then in {y}'s project configuration you could specify:
Related Issue(s)
Fixes #