fix: evaluate all patterns before throwing EDUPLICATEWORKSPACE#32
Merged
fix: evaluate all patterns before throwing EDUPLICATEWORKSPACE#32
EDUPLICATEWORKSPACE#32Conversation
louis-bompart
commented
Mar 10, 2022
| ) | ||
| }) | ||
|
|
||
| test('match duplicates then exclude one', t => { |
Contributor
Author
There was a problem hiding this comment.
This spec was ❌ prior to my change, the others (apart from the one checking the error message construction) were working already. I decided to include them nonetheless to help define the behavior of the function for the public.
11be2cd to
da4dd70
Compare
wraithgar
reviewed
Mar 10, 2022
da4dd70 to
6aa21ad
Compare
6aa21ad to
ba05e18
Compare
wraithgar
approved these changes
Mar 10, 2022
Member
|
Thanks for all the PRs lately! |
Member
|
We are going to get this into today's npm release for you |
Contributor
Author
oh wow. I was not expecting that much 🎉 Thanks a bunch :D |
Contributor
|
Merci bcp @louis-bompart! it's out now in |
This file contains hidden or 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
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.
Motivation
Currently, if duplicates packages are included by the
n-th pattern of theworkspacesfield of apackage.json, the algorithm will throw anEDUPLICATEWORKSPACEwithout evaluating the other patterns.So, for example, if the
n+1-th pattern excludes all the duplicates, the workspaces should be considered valid.Reflexion
This is I think important because the
workspacesfield is described as an array of file patterns 1.The most historic occurrence to my knowledge of 'file pattern' for the package.json is the
filesfield, and the behavior of those patterns is described as such:The Git documentation explains that the '!' prefix is like this:
I think that the keyword here is 'previous'. This means the patterns do need to be evaluated sequentially (so for example, we cannot leverage the
ignoreoptions ofglob, otherwise subsequent inclusion would not work).Proposed solution
Short: Evaluate all patterns sequentially and check for duplicates at the end. List all duplicates on error.
Longish:
We evaluate the patterns one by one. At each match, we either store or remove the path from a Set associated with the packageName, depending on if the pattern is an exclusive or inclusive one.
Once that's done, we got a
Map<Name,Set<PackagePath>>.We go through the Map entries to evaluate the presence of duplicates. If there's none for the current entries, we copy its key and the sole value of its associated Set to the result map (if the Set is empty, we skip the entry).
If there's a duplicate, we generate an error message stub and append it to the error array.
Here's that a choice I made:
Either we 'fail-fast' and provide only the first duplicate pair.
Either fail at the end and provide all the duplicates.
My personal preference leans towards the exhaustive error message, so I used it in this PR.
Finally, if the error array is not empty (ish, we check
length>1to ignore the 'header' of the error message), throw anEDUPLICATEWORKSPACEwith a message from the concatenated error array. If there's no error, return the result arrayReferences
Footnotes
https://docs.npmjs.com/cli/v8/configuring-npm/package-json#workspaces ↩
https://docs.npmjs.com/cli/v8/configuring-npm/package-json#files ↩
https://git-scm.com/docs/gitignore ↩