fix(resolution): Prevent duplicate patterns in PackageResolver patternsByPackage array #5681
Conversation
…nsByPackage array Previously duplicate patterns would be inserted into this array, causing unneccecary looping and manifest reading of the same patterns. It also caused a bug when duplicate patterns were marked as peerDeps (yarnpkg#5676). We now prevent these duplicate entries. yarnpkg#5676
|
__tests__/package-resolver.js
Outdated
|
|
||
| const config = await Config.create( | ||
| const config = await Config.create( | ||
| Object.assign( |
There was a problem hiding this comment.
nit: prefer object spread notation:
{
cwd: loc,
offline: false,
cacheFolder,
...configOptions,
}| @@ -352,7 +353,9 @@ export default class PackageResolver { | |||
| this.patterns[pattern] = info; | |||
|
|
|||
| const byName = (this.patternsByPackage[info.name] = this.patternsByPackage[info.name] || []); | |||
| byName.push(pattern); | |||
| if (byName.indexOf(pattern) === -1) { | |||
There was a problem hiding this comment.
I feel like it would be a lot more efficient to use a Set here since the ordering doesn't seem to be important?
I guess that'd be a refactor we need to do in a follow up?
There was a problem hiding this comment.
In that specific function yes, Set would be better, but the rest of the functions use Array.map and Array.filter on that object, which are not available on Set so we would have to convert Set to Array all over the place: [...patterns].map( which is probably worse overall for memory and performance usage. (expanding the set to an array frequently, vs infrequent .indexOf calls on small arrays)
There was a problem hiding this comment.
In that specific function yes, Set would be better, but the rest of the functions use Array.map and Array.filter on that object, which are not available on Set so we would have to convert Set to Array all over the place: [...patterns].map( which is probably worse overall for memory and performance usage. (expanding the set to an array frequently, vs infrequent .indexOf calls on small arrays)
Shameless plug: https://github.com/BYK/superset
|
Somehow all the CI environments now fail for a snapshot that would not have changed based on my last commit. Cool... 😠 This is due to a new version of |
|
@rally25rs submitted #5687 to fix the failures on master. |
Summary
Fixes #5676. Previously duplicate patterns would be inserted into this array, causing unnecessary looping and manifest reading of the same patterns. It also caused a bug when duplicate patterns were marked as
peerDeps. We now prevent these duplicate entries.
Test plan
Added test to
package-resolvertests.