fix: evaluate all patterns before throwing EDUPLICATEWORKSPACE

[louis-bompart]

Motivation

Currently, if duplicates packages are included by the n-th pattern of the workspaces field of a package.json, the algorithm will throw an EDUPLICATEWORKSPACE without 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 workspaces field is described as an array of file patterns ¹.
The most historic occurrence to my knowledge of 'file pattern' for the package.json is the files field, and the behavior of those patterns is described as such:

File patterns follow a similar syntax to .gitignore ²

The Git documentation explains that the '!' prefix is like this:

An optional prefix "!" which negates the pattern; any matching file excluded by a previous pattern will become included again ³

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 ignore options of glob, 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.

  • Pro: Faster to fail
  • Con: If the end-user has more than one duplicate, it will need to repeat the operation.

• Either fail at the end and provide all the duplicates.

  • Pro: Exhaustive error, the user has all the information to go from ❌ to ✔️ in just 1 run.
  • Con: Slower to fail.

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>1 to ignore the 'header' of the error message), throw an EDUPLICATEWORKSPACE with a message from the concatenated error array. If there's no error, return the result array

References

Footnotes

1. https://docs.npmjs.com/cli/v8/configuring-npm/package-json#workspaces ↩

2. https://docs.npmjs.com/cli/v8/configuring-npm/package-json#files ↩

3. https://git-scm.com/docs/gitignore ↩

[louis-bompart]

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.

[wraithgar]

Thanks for all the PRs lately!

[wraithgar]

We are going to get this into today's npm release for you

[louis-bompart]

We are going to get this into today's npm release for you

oh wow. I was not expecting that much 🎉 Thanks a bunch :D

[ruyadorno]

Merci bcp @louis-bompart! it's out now in npm@8.5.4 🚀