fix: faster workspace mapping (#143)

This is a cleanup of #138 to
get code coverage finished in the tests.

Credit: @thecodrr

---------

Co-authored-by: Abdullah Atta <abdullahatta@streetwriters.co>

lib/index.js

@@ -5,9 +5,10 @@ const { minimatch } = require('minimatch')
 const rpj = require('read-package-json-fast')
 const { glob } = require('glob')
 
-function appendNegatedPatterns (patterns) {
- const results = []
- for (let pattern of patterns) {
+function appendNegatedPatterns (allPatterns) {
+ const patterns = []
+ const negatedPatterns = []
+ for (let pattern of allPatterns) {
  const excl = pattern.match(/^!+/)
  if (excl) {
  pattern = pattern.slice(excl[0].length)
@@ -18,10 +19,35 @@ function appendNegatedPatterns (patterns) {
 
  // an odd number of ! means a negated pattern. !!foo ==> foo
  const negate = excl && excl[0].length % 2 === 1
- results.push({ pattern, negate })
+ if (negate) {
+ negatedPatterns.push(pattern)
+ } else {
+ // remove negated patterns that appeared before this pattern to avoid
+ // ignoring paths that were matched afterwards
+ // e.g: ['packages/**', '!packages/b/**', 'packages/b/a']
+ // in the above list, the last pattern overrides the negated pattern
+ // right before it. In effect, the above list would become:
+ // ['packages/**', 'packages/b/a']
+ // The order matters here which is why we must do it inside the loop
+ // as opposed to doing it all together at the end.
+ for (let i = 0; i < negatedPatterns.length; ++i) {
+ const negatedPattern = negatedPatterns[i]
+ if (minimatch(pattern, negatedPattern)) {
+ negatedPatterns.splice(i, 1)
+ }
+ }
+ patterns.push(pattern)
+ }
  }
 
- return results
+ // use the negated patterns to eagerly remove all the patterns that
+ // can be removed to avoid unnecessary crawling
+ for (const negated of negatedPatterns) {
+ for (const pattern of minimatch.match(patterns, negated)) {
+ patterns.splice(patterns.indexOf(pattern), 1)
+ }
+ }
+ return { patterns, negatedPatterns }
 }
 
 function getPatterns (workspaces) {
@@ -77,64 +103,66 @@ async function mapWorkspaces (opts = {}) {
  }
 
  const { workspaces = [] } = opts.pkg
- const patterns = getPatterns(workspaces)
+ const { patterns, negatedPatterns } = getPatterns(workspaces)
  const results = new Map()
  const seen = new Map()
 
- if (!patterns.length) {
+ if (!patterns.length && !negatedPatterns.length) {
  return results
  }
 
  const getGlobOpts = () => ({
  ...opts,
  ignore: [
  ...opts.ignore || [],
- ...['**/node_modules/**'],
+ '**/node_modules/**',
+ // just ignore the negated patterns to avoid unnecessary crawling
+ ...negatedPatterns,
  ],
  })
 
  const getPackagePathname = pkgPathmame(opts)
 
- for (const item of patterns) {
- let matches = await glob(getGlobPattern(item.pattern), getGlobOpts())
- // preserves glob@8 behavior
- matches = matches.sort((a, b) => a.localeCompare(b, 'en'))
-
- for (const match of matches) {
- let pkg
- const packageJsonPathname = getPackagePathname(match, 'package.json')
- const packagePathname = path.dirname(packageJsonPathname)
-
- try {
- pkg = await rpj(packageJsonPathname)
- } catch (err) {
- if (err.code === 'ENOENT') {
- continue
- } else {
- throw err
- }
- }
+ let matches = await glob(patterns.map((p) => getGlobPattern(p)), getGlobOpts())
+ // preserves glob@8 behavior
+ matches = matches.sort((a, b) => a.localeCompare(b, 'en'))
+
+ // we must preserve the order of results according to the given list of
+ // workspace patterns
+ const orderedMatches = []
+ for (const pattern of patterns) {
+ orderedMatches.push(...matches.filter((m) => {
+ return minimatch(m, pattern, { partial: true, windowsPathsNoEscape: true })
+ }))
+ }
 
- const name = getPackageName(pkg, packagePathname)
+ for (const match of orderedMatches) {
+ let pkg
+ const packageJsonPathname = getPackagePathname(match, 'package.json')
 
- let seenPackagePathnames = seen.get(name)
- if (!seenPackagePathnames) {
- seenPackagePathnames = new Set()
- seen.set(name, seenPackagePathnames)
- }
- if (item.negate) {
- seenPackagePathnames.delete(packagePathname)
+ try {
+ pkg = await rpj(packageJsonPathname)
+ } catch (err) {
+ if (err.code === 'ENOENT') {
+ continue
  } else {
- seenPackagePathnames.add(packagePathname)
+ throw err
  }
  }
+
+ const packagePathname = path.dirname(packageJsonPathname)
+ const name = getPackageName(pkg, packagePathname)
+
+ let seenPackagePathnames = seen.get(name)
+ if (!seenPackagePathnames) {
+ seenPackagePathnames = new Set()
+ seen.set(name, seenPackagePathnames)
+ }
+ seenPackagePathnames.add(packagePathname)
  }
 
  const errorMessageArray = ['must not have multiple workspaces with the same name']
  for (const [packageName, seenPackagePathnames] of seen) {
- if (seenPackagePathnames.size === 0) {
- continue
- }
  if (seenPackagePathnames.size > 1) {
  addDuplicateErrorMessages(errorMessageArray, packageName, seenPackagePathnames)
  } else {
@@ -177,30 +205,25 @@ mapWorkspaces.virtual = function (opts = {}) {
  const { workspaces = [] } = packages[''] || {}
  // uses a pathname-keyed map in order to negate the exact items
  const results = new Map()
- const patterns = getPatterns(workspaces)
- if (!patterns.length) {
+ const { patterns, negatedPatterns } = getPatterns(workspaces)
+ if (!patterns.length && !negatedPatterns.length) {
  return results
  }
- patterns.push({ pattern: '**/node_modules/**', negate: true })
-
- const getPackagePathname = pkgPathmame(opts)
+ negatedPatterns.push('**/node_modules/**')
 
- for (const packageKey of Object.keys(packages)) {
- if (packageKey === '') {
- continue
+ const packageKeys = Object.keys(packages)
+ for (const pattern of negatedPatterns) {
+ for (const packageKey of minimatch.match(packageKeys, pattern)) {
+ packageKeys.splice(packageKeys.indexOf(packageKey), 1)
  }
+ }
 
- for (const item of patterns) {
- if (minimatch(packageKey, item.pattern)) {
- const packagePathname = getPackagePathname(packageKey)
- const name = getPackageName(packages[packageKey], packagePathname)
-
- if (item.negate) {
- results.delete(packagePathname)
- } else {
- results.set(packagePathname, name)
- }
- }
+ const getPackagePathname = pkgPathmame(opts)
+ for (const pattern of patterns) {
+ for (const packageKey of minimatch.match(packageKeys, pattern)) {
+ const packagePathname = getPackagePathname(packageKey)
+ const name = getPackageName(packages[packageKey], packagePathname)
+ results.set(packagePathname, name)
  }
  }
 

tap-snapshots/test/test.js.test.cjs

@@ -11,9 +11,9 @@ Map {
 }
 `
 
-exports[`test/test.js TAP double negate patterns > should include doubly-negated items into resulting map 1`] = `
+exports[`test/test.js TAP double negated patterns > should include doubly-negated items into resulting map 1`] = `
 Map {
- "a" => "{CWD}/test/tap-testdir-test-double-negate-patterns/packages/a",
+ "a" => "{CWD}/test/tap-testdir-test-double-negated-patterns/packages/a",
 }
 `
 
@@ -78,13 +78,13 @@ package 'b' has conflicts in the following paths:
 }
 `
 
-exports[`test/test.js TAP multiple negate patterns > should not include any negated pattern 1`] = `
+exports[`test/test.js TAP multiple negated patterns > should not include any negated pattern 1`] = `
 Map {}
 `
 
-exports[`test/test.js TAP negate pattern > should not include negated patterns 1`] = `
+exports[`test/test.js TAP negated pattern > should not include negated patterns 1`] = `
 Map {
- "a" => "{CWD}/test/tap-testdir-test-negate-pattern/packages/a",
+ "a" => "{CWD}/test/tap-testdir-test-negated-pattern/packages/a",
 }
 `
 
@@ -114,6 +114,12 @@ Map {
 }
 `
 
+exports[`test/test.js TAP overlapping negated pattern > should not include negated patterns 1`] = `
+Map {
+ "a" => "{CWD}/test/tap-testdir-test-overlapping-negated-pattern/packages/a",
+}
+`
+
 exports[`test/test.js TAP root declared within workspaces > should allow the root package to be declared within workspaces 1`] = `
 Map {
  "a" => "{CWD}/test/tap-testdir-test-root-declared-within-workspaces/packages/a",
@@ -135,7 +141,7 @@ Map {
 }
 `
 
-exports[`test/test.js TAP triple negate patterns > should exclude thrice-negated items from resulting map 1`] = `
+exports[`test/test.js TAP triple negated patterns > should exclude thrice-negated items from resulting map 1`] = `
 Map {}
 `
 

test/test.js

@@ -649,7 +649,7 @@ test('ignore option', t => {
  )
 })
 
-test('negate pattern', t => {
+test('negated pattern', t => {
  const cwd = t.testdir({
  foo: {
  bar: {
@@ -692,7 +692,7 @@ test('negate pattern', t => {
  )
 })
 
-test('multiple negate patterns', t => {
+test('multiple negated patterns', t => {
  const cwd = t.testdir({
  foo: {
  bar: {
@@ -737,7 +737,51 @@ test('multiple negate patterns', t => {
  )
 })
 
-test('double negate patterns', t => {
+test('overlapping negated pattern', t => {
+ const cwd = t.testdir({
+ foo: {
+ bar: {
+ baz: {
+ e: {
+ 'package.json': '{ "name": "e" }',
+ },
+ },
+ node_modules: {
+ b: {
+ 'package.json': '{ "name": "b" }',
+ },
+ },
+ },
+ },
+ packages: {
+ a: {
+ 'package.json': '{ "name": "a" }',
+ node_modules: {
+ c: {
+ 'package.json': '{ "name": "c" }',
+ },
+ },
+ },
+ },
+ })
+
+ return t.resolveMatchSnapshot(
+ mapWorkspaces({
+ cwd,
+ pkg: {
+ workspaces: [
+ 'packages/*',
+ 'foo/**',
+ '**/baz/**',
+ '!**/baz/**',
+ ],
+ },
+ }),
+ 'should not include negated patterns'
+ )
+})
+
+test('double negated patterns', t => {
  const cwd = t.testdir({
  packages: {
  a: {
@@ -759,7 +803,7 @@ test('double negate patterns', t => {
  )
 })
 
-test('triple negate patterns', t => {
+test('triple negated patterns', t => {
  const cwd = t.testdir({
  packages: {
  a: {