From 64a7c0aa01839224e7bc09e11533ad6235ef2cc2 Mon Sep 17 00:00:00 2001 From: Adam Skoufis Date: Fri, 19 Apr 2024 10:41:35 +1000 Subject: [PATCH] Replace `fast-glob` with `fdir` and `picomatch` --- .changeset/nervous-carpets-call.md | 7 ++ .changeset/orange-laws-perform.md | 5 + .changeset/young-seals-shave.md | 5 + fixtures/multiple-routes/package.json | 3 +- .../sku/context/defaultCompilePackages.js | 99 +++++++++++-------- packages/sku/lib/validateLessFiles.js | 12 ++- packages/sku/lib/validatePeerDeps.js | 52 ++++++---- packages/sku/package.json | 4 +- packages/sku/scripts/init.js | 10 +- pnpm-lock.yaml | 60 +++++------ 10 files changed, 159 insertions(+), 98 deletions(-) create mode 100644 .changeset/nervous-carpets-call.md create mode 100644 .changeset/orange-laws-perform.md create mode 100644 .changeset/young-seals-shave.md diff --git a/.changeset/nervous-carpets-call.md b/.changeset/nervous-carpets-call.md new file mode 100644 index 000000000..57ea995b4 --- /dev/null +++ b/.changeset/nervous-carpets-call.md @@ -0,0 +1,7 @@ +--- +'sku': patch +--- + +Disable peer dependency validation for PNPM + +The method we currently use to validate peer dependencies and warn users about duplicate package is not compatible with how PNPM organizes dependes in `node_modules`. This feature has been disabled for PNPM repos until further notice. diff --git a/.changeset/orange-laws-perform.md b/.changeset/orange-laws-perform.md new file mode 100644 index 000000000..25ac9bd4f --- /dev/null +++ b/.changeset/orange-laws-perform.md @@ -0,0 +1,5 @@ +--- +'sku': patch +--- + +Replace `fast-glob` with `fdir` and `picomatch` diff --git a/.changeset/young-seals-shave.md b/.changeset/young-seals-shave.md new file mode 100644 index 000000000..c93ec8478 --- /dev/null +++ b/.changeset/young-seals-shave.md @@ -0,0 +1,5 @@ +--- +'sku': patch +--- + +Improve performance of peer dependency validation file system crawling diff --git a/fixtures/multiple-routes/package.json b/fixtures/multiple-routes/package.json index b779b4504..6b4a7daaa 100644 --- a/fixtures/multiple-routes/package.json +++ b/fixtures/multiple-routes/package.json @@ -11,6 +11,5 @@ "@testing-library/react": "^14.0.0", "dedent": "^1.5.1", "sku": "workspace:*" - }, - "skuSkipValidatePeerDeps": true + } } diff --git a/packages/sku/context/defaultCompilePackages.js b/packages/sku/context/defaultCompilePackages.js index a99e5ef65..0816b66e8 100644 --- a/packages/sku/context/defaultCompilePackages.js +++ b/packages/sku/context/defaultCompilePackages.js @@ -1,8 +1,8 @@ const { posix: path } = require('node:path'); const chalk = require('chalk'); -const glob = require('fast-glob'); +const { fdir: Fdir } = require('fdir'); -const { cwd: skuCwd } = require('../lib/cwd'); +const { cwd } = require('../lib/cwd'); const toPosixPath = require('../lib/toPosixPath'); const { rootDir, isPnpm } = require('../lib/packageManager'); @@ -10,46 +10,67 @@ const { rootDir, isPnpm } = require('../lib/packageManager'); /** @type {string[]} */ let detectedCompilePackages = []; -try { - const globs = ['node_modules/@seek/*/package.json']; - const cwd = skuCwd(); +// If there's no rootDir, we're either inside `sku init`, or we can't determine the user's +// package manager. In either case, we can't correctly detect compile packages. +if (rootDir) { + try { + let crawler = new Fdir(); - if (isPnpm && rootDir) { - const pnpmVirtualStorePath = path.join( - toPosixPath(rootDir), - 'node_modules/.pnpm', - ); - const pnpmVirtualStoreRelativePath = path.relative( - '.', - pnpmVirtualStorePath, - ); - const pnpmVirtualStoreGlob = path.join( - pnpmVirtualStoreRelativePath, - '@seek*/node_modules/@seek/*/package.json', - ); + if (isPnpm) { + // Follow symlinks inside node_modules into the pnpm virtual store + crawler = crawler.withSymlinks().withRelativePaths(); + } else { + crawler = crawler.withBasePath(); + } - globs.push(pnpmVirtualStoreGlob); - } + let results = crawler + .filter((file) => file.endsWith('package.json')) + .crawl('./node_modules/@seek') + .sync(); + + if (isPnpm) { + const pnpmVirtualStorePath = path.join( + toPosixPath(rootDir), + 'node_modules/.pnpm', + ); + + const pnpmVirtualStoreRelativePath = path.relative( + '.', + pnpmVirtualStorePath, + ); - detectedCompilePackages = glob - .sync(globs, { - cwd, - }) - .map((packagePath) => { - const packageJson = require(path.join(cwd, packagePath)); - - return { - isCompilePackage: Boolean(packageJson.skuCompilePackage), - packageName: packageJson.name, - }; - }) - .filter(({ isCompilePackage }) => isCompilePackage) - .map(({ packageName }) => packageName); -} catch (e) { - console.log( - chalk.red`Warning: Failed to detect compile packages. Contact #sku-support.`, - ); - console.error(e); + const pnpmVirtualStoreResults = new Fdir() + .withRelativePaths() + .glob('@seek*/node_modules/@seek/*/package.json') + .crawl(pnpmVirtualStoreRelativePath) + .sync(); + + results.push(...pnpmVirtualStoreResults); + + // All results will be relative to the virtual store directory, so we need + // to prepend the relative path from the current directory to the virtual store + results = results.map((file) => + path.join(pnpmVirtualStoreRelativePath, file), + ); + } + + detectedCompilePackages = results + .map((packagePath) => { + const packageJson = require(path.join(cwd(), packagePath)); + + return { + isCompilePackage: Boolean(packageJson.skuCompilePackage), + packageName: packageJson.name, + }; + }) + .filter(({ isCompilePackage }) => isCompilePackage) + .map(({ packageName }) => packageName); + } catch (e) { + console.log( + chalk.red`Warning: Failed to detect compile packages. Contact #sku-support.`, + ); + console.error(e); + } } module.exports = [ diff --git a/packages/sku/lib/validateLessFiles.js b/packages/sku/lib/validateLessFiles.js index 3c954ed6a..a47a0e73c 100644 --- a/packages/sku/lib/validateLessFiles.js +++ b/packages/sku/lib/validateLessFiles.js @@ -1,7 +1,6 @@ const fs = require('node:fs'); -const path = require('node:path'); -const glob = require('fast-glob'); const chalk = require('chalk'); +const { fdir: Fdir } = require('fdir'); const printBanner = require('./banner'); const exists = require('./exists'); @@ -14,11 +13,18 @@ module.exports = async () => { const lessFileGlobResults = await Promise.all( srcPathsExist .filter((srcPath) => srcPath && fs.statSync(srcPath).isDirectory()) - .map(async (srcPath) => await glob(path.join(srcPath, '**/*.less'))), + .map(async (srcPath) => + new Fdir() + .filter((file) => file.endsWith('.less')) + .crawl(srcPath) + .withPromise(), + ), ); + const srcHasLessFiles = lessFileGlobResults.some( (fileArray) => fileArray.length > 0, ); + if (srcHasLessFiles) { printBanner('warning', 'LESS styles detected', [ `Support for ${chalk.bold('LESS')} has been deprecated.`, diff --git a/packages/sku/lib/validatePeerDeps.js b/packages/sku/lib/validatePeerDeps.js index 6f9afc15e..d0a8c63b6 100644 --- a/packages/sku/lib/validatePeerDeps.js +++ b/packages/sku/lib/validatePeerDeps.js @@ -1,13 +1,13 @@ -const { getWhyCommand } = require('./packageManager'); +const { getWhyCommand, isPnpm } = require('./packageManager'); const { readFile } = require('node:fs/promises'); -const glob = require('fast-glob'); +const { fdir: Fdir } = require('fdir'); const semver = require('semver'); const chalk = require('chalk'); const banner = require('./banner'); const track = require('../telemetry'); -const { cwd, getPathFromCwd } = require('../lib/cwd'); +const { getPathFromCwd } = require('../lib/cwd'); const { paths } = require('../context'); /** @@ -21,30 +21,44 @@ const asyncMap = (list, fn) => { const singletonPackages = ['@vanilla-extract/css']; module.exports = async () => { + if (isPnpm) { + // pnpm doesn't nest dependencies in the same way as yarn or npm, so the method used below won't + // work for detecting duplicate packages + return; + } + try { + /** @type {string[]} */ const packages = []; - for (const packageName of [ - ...paths.compilePackages, - ...singletonPackages, - ]) { - const results = await glob( - [ - `node_modules/${packageName}/package.json`, - `node_modules/**/node_modules/${packageName}/package.json`, - ], - { - cwd: cwd(), - }, + const packagesToCheck = [...paths.compilePackages, ...singletonPackages]; + const packageNameByPattern = Object.fromEntries( + packagesToCheck.map((packageName) => [ + `node_modules/${packageName}/package.json`, + packageName, + ]), + ); + + const patterns = Object.keys(packageNameByPattern); + + const results = await new Fdir() + .withBasePath() + .filter((file) => patterns.some((pattern) => file.endsWith(pattern))) + .crawl('node_modules') + .withPromise(); + + for (const [pattern, packageName] of Object.entries(packageNameByPattern)) { + const resultsForPackage = results.filter((result) => + result.endsWith(pattern), ); - if (results.length > 1) { + if (resultsForPackage.length > 1) { const messages = [ chalk`Multiple copies of {bold ${packageName}} are present in node_modules. This is likely to cause errors, but even if it works, it will probably result in an unnecessarily large bundle size.`, ]; messages.push( - results + resultsForPackage .map((depLocation) => { const { version } = require(getPathFromCwd(depLocation)); @@ -64,9 +78,9 @@ module.exports = async () => { compile_package: packageName, }); banner('error', 'Error: Duplicate packages detected', messages); - } - packages.push(...results); + packages.push(...resultsForPackage); + } } const compilePackages = new Map(); diff --git a/packages/sku/package.json b/packages/sku/package.json index 3b197a993..bb77accff 100644 --- a/packages/sku/package.json +++ b/packages/sku/package.json @@ -85,8 +85,8 @@ "eslint-config-seek": "^12.0.1", "exception-formatter": "^2.1.2", "express": "^4.16.3", - "fast-glob": "^3.2.5", "fastest-validator": "^1.9.0", + "fdir": "^6.1.1", "find-up": "^5.0.0", "get-port": "^5.0.0", "hostile": "^1.3.3", @@ -104,6 +104,7 @@ "node-html-parser": "^6.1.1", "open": "^7.3.1", "path-to-regexp": "^6.2.0", + "picomatch": "^3.0.1", "postcss": "^8.4.0", "postcss-loader": "^8.0.0", "prettier": "^2.8.8", @@ -133,6 +134,7 @@ "@types/debug": "^4.1.12", "@types/express": "^4.17.11", "@types/minimist": "^1.2.5", + "@types/picomatch": "^2.3.3", "@types/react": "^18.2.3", "@types/react-dom": "^18.2.3", "@types/which": "^3.0.0", diff --git a/packages/sku/scripts/init.js b/packages/sku/scripts/init.js index 6c153b397..6d10b9b3a 100644 --- a/packages/sku/scripts/init.js +++ b/packages/sku/scripts/init.js @@ -21,7 +21,7 @@ const install = require('../lib/install'); const banner = require('../lib/banner'); const toPosixPath = require('../lib/toPosixPath'); const trace = require('debug')('sku:init'); -const glob = require('fast-glob'); +const { fdir: Fdir } = require('fdir'); const ejs = require('ejs'); const args = require('../config/args'); @@ -152,9 +152,11 @@ const getTemplateFileDestinationFromRoot = process.chdir(root); const templateDirectory = path.join(toPosixPath(__dirname), '../template'); - const templateFiles = await glob(`${templateDirectory}/**/*`, { - onlyFiles: true, - }); + + const templateFiles = await new Fdir() + .withBasePath() + .crawl(templateDirectory) + .withPromise(); const getTemplateFileDestination = getTemplateFileDestinationFromRoot( root, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a9c316884..bbb80e65f 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -665,12 +665,12 @@ importers: express: specifier: ^4.16.3 version: 4.19.2 - fast-glob: - specifier: ^3.2.5 - version: 3.3.2 fastest-validator: specifier: ^1.9.0 version: 1.17.0 + fdir: + specifier: ^6.1.1 + version: 6.1.1(picomatch@3.0.1) find-up: specifier: ^5.0.0 version: 5.0.0 @@ -722,6 +722,9 @@ importers: path-to-regexp: specifier: ^6.2.0 version: 6.2.1 + picomatch: + specifier: ^3.0.1 + version: 3.0.1 postcss: specifier: ^8.4.0 version: 8.4.38 @@ -804,6 +807,9 @@ importers: '@types/minimist': specifier: ^1.2.5 version: 1.2.5 + '@types/picomatch': + specifier: ^2.3.3 + version: 2.3.3 '@types/react': specifier: ^18.2.3 version: 18.2.73 @@ -2579,7 +2585,6 @@ packages: cpu: [ppc64] os: [aix] requiresBuild: true - dev: false optional: true /@esbuild/aix-ppc64@0.20.2: @@ -2605,7 +2610,6 @@ packages: cpu: [arm64] os: [android] requiresBuild: true - dev: false optional: true /@esbuild/android-arm64@0.20.2: @@ -2631,7 +2635,6 @@ packages: cpu: [arm] os: [android] requiresBuild: true - dev: false optional: true /@esbuild/android-arm@0.20.2: @@ -2657,7 +2660,6 @@ packages: cpu: [x64] os: [android] requiresBuild: true - dev: false optional: true /@esbuild/android-x64@0.20.2: @@ -2683,7 +2685,6 @@ packages: cpu: [arm64] os: [darwin] requiresBuild: true - dev: false optional: true /@esbuild/darwin-arm64@0.20.2: @@ -2709,7 +2710,6 @@ packages: cpu: [x64] os: [darwin] requiresBuild: true - dev: false optional: true /@esbuild/darwin-x64@0.20.2: @@ -2735,7 +2735,6 @@ packages: cpu: [arm64] os: [freebsd] requiresBuild: true - dev: false optional: true /@esbuild/freebsd-arm64@0.20.2: @@ -2761,7 +2760,6 @@ packages: cpu: [x64] os: [freebsd] requiresBuild: true - dev: false optional: true /@esbuild/freebsd-x64@0.20.2: @@ -2787,7 +2785,6 @@ packages: cpu: [arm64] os: [linux] requiresBuild: true - dev: false optional: true /@esbuild/linux-arm64@0.20.2: @@ -2813,7 +2810,6 @@ packages: cpu: [arm] os: [linux] requiresBuild: true - dev: false optional: true /@esbuild/linux-arm@0.20.2: @@ -2839,7 +2835,6 @@ packages: cpu: [ia32] os: [linux] requiresBuild: true - dev: false optional: true /@esbuild/linux-ia32@0.20.2: @@ -2865,7 +2860,6 @@ packages: cpu: [loong64] os: [linux] requiresBuild: true - dev: false optional: true /@esbuild/linux-loong64@0.20.2: @@ -2891,7 +2885,6 @@ packages: cpu: [mips64el] os: [linux] requiresBuild: true - dev: false optional: true /@esbuild/linux-mips64el@0.20.2: @@ -2917,7 +2910,6 @@ packages: cpu: [ppc64] os: [linux] requiresBuild: true - dev: false optional: true /@esbuild/linux-ppc64@0.20.2: @@ -2943,7 +2935,6 @@ packages: cpu: [riscv64] os: [linux] requiresBuild: true - dev: false optional: true /@esbuild/linux-riscv64@0.20.2: @@ -2969,7 +2960,6 @@ packages: cpu: [s390x] os: [linux] requiresBuild: true - dev: false optional: true /@esbuild/linux-s390x@0.20.2: @@ -2995,7 +2985,6 @@ packages: cpu: [x64] os: [linux] requiresBuild: true - dev: false optional: true /@esbuild/linux-x64@0.20.2: @@ -3021,7 +3010,6 @@ packages: cpu: [x64] os: [netbsd] requiresBuild: true - dev: false optional: true /@esbuild/netbsd-x64@0.20.2: @@ -3047,7 +3035,6 @@ packages: cpu: [x64] os: [openbsd] requiresBuild: true - dev: false optional: true /@esbuild/openbsd-x64@0.20.2: @@ -3073,7 +3060,6 @@ packages: cpu: [x64] os: [sunos] requiresBuild: true - dev: false optional: true /@esbuild/sunos-x64@0.20.2: @@ -3099,7 +3085,6 @@ packages: cpu: [arm64] os: [win32] requiresBuild: true - dev: false optional: true /@esbuild/win32-arm64@0.20.2: @@ -3125,7 +3110,6 @@ packages: cpu: [ia32] os: [win32] requiresBuild: true - dev: false optional: true /@esbuild/win32-ia32@0.20.2: @@ -3151,7 +3135,6 @@ packages: cpu: [x64] os: [win32] requiresBuild: true - dev: false optional: true /@esbuild/win32-x64@0.20.2: @@ -5530,6 +5513,10 @@ packages: /@types/parse-json@4.0.2: resolution: {integrity: sha512-dISoDXWWQwUquiKsyZ4Ng+HX2KsPL7LyHKHQwgGFEA3IaKac4Obd+h2a/a6waisAoepJlBcx9paWqjA8/HVjCw==} + /@types/picomatch@2.3.3: + resolution: {integrity: sha512-Yll76ZHikRFCyz/pffKGjrCwe/le2CDwOP5F210KQo27kpRE46U2rDnzikNlVn6/ezH3Mhn46bJMTfeVTtcYMg==} + dev: true + /@types/pretty-hrtime@1.0.3: resolution: {integrity: sha512-nj39q0wAIdhwn7DGUyT9irmsKK1tV0bd5WFEhgpqNTMFZ8cE+jieuTphCW0tfdm47S2zVT5mr09B28b1chmQMA==} @@ -8660,7 +8647,6 @@ packages: '@esbuild/win32-arm64': 0.19.12 '@esbuild/win32-ia32': 0.19.12 '@esbuild/win32-x64': 0.19.12 - dev: false /esbuild@0.20.2: resolution: {integrity: sha512-WdOOppmUNU+IbZ0PaDiTst80zjnrOkyJNHoKupIcVyU8Lvla3Ugx94VzkQ32Ijqd7UhHJy75gNWDMUekcrSJ6g==} @@ -9312,6 +9298,17 @@ packages: dependencies: pend: 1.2.0 + /fdir@6.1.1(picomatch@3.0.1): + resolution: {integrity: sha512-QfKBVg453Dyn3mr0Q0O+Tkr1r79lOTAKSi9f/Ot4+qVEwxWhav2Z+SudrG9vQjM2aYRMQQZ2/Q1zdA8ACM1pDg==} + peerDependencies: + picomatch: 3.x + peerDependenciesMeta: + picomatch: + optional: true + dependencies: + picomatch: 3.0.1 + dev: false + /fetch-retry@5.0.6: resolution: {integrity: sha512-3yurQZ2hD9VISAhJJP9bpYFNQrHHBXE2JxxjY5aLEcDi46RmAzJE2OC9FAde0yis5ElW0jTTzs0zfg/Cca4XqQ==} dev: false @@ -12829,6 +12826,11 @@ packages: resolution: {integrity: sha512-JU3teHTNjmE2VCGFzuY8EXzCDVwEqB2a8fsIvwaStHhAWJEeVd1o1QD80CU6+ZdEXXSLbSsuLwJjkCBWqRQUVA==} engines: {node: '>=8.6'} + /picomatch@3.0.1: + resolution: {integrity: sha512-I3EurrIQMlRc9IaAZnqRR044Phh2DXY+55o7uJ0V+hYZAcQYSuFWsc9q5PvyDHUSCe1Qxn/iBz+78s86zWnGag==} + engines: {node: '>=10'} + dev: false + /pify@2.3.0: resolution: {integrity: sha512-udgsAY+fTnvv7kI7aaxbqwWNb0AHiB0qBO89PZKPkoTmGOgdbrHDKD+0B2X4uTfJ/FT1R09r9gTsjUjNJotuog==} engines: {node: '>=0.10.0'} @@ -15013,7 +15015,6 @@ packages: serialize-javascript: 6.0.2 terser: 5.30.0 webpack: 5.91.0(@swc/core@1.4.11)(esbuild@0.19.12) - dev: false /terser-webpack-plugin@5.3.10(@swc/core@1.4.11)(webpack@5.91.0): resolution: {integrity: sha512-BKFPWlPDndPs+NGGCr1U59t0XScL5317Y0UReNrHaw9/FwhPENlq6bfgs+4yPfyP51vqC1bQ4rp1EfXW5ZSH9w==} @@ -15762,7 +15763,7 @@ packages: on-finished: 2.4.1 range-parser: 1.2.1 schema-utils: 4.2.0 - webpack: 5.91.0(@swc/core@1.4.11)(webpack-cli@5.1.4) + webpack: 5.91.0(@swc/core@1.4.11)(esbuild@0.19.12) /webpack-dev-server@5.0.4(debug@4.3.4)(webpack-cli@5.1.4)(webpack@5.91.0): resolution: {integrity: sha512-dljXhUgx3HqKP2d8J/fUMvhxGhzjeNVarDLcbO/EWMSgRizDkxHQDZQaLFL5VJY9tRBj2Gz+rvCEYYvhbqPHNA==} @@ -15937,7 +15938,6 @@ packages: - '@swc/core' - esbuild - uglify-js - dev: false /webpack@5.91.0(@swc/core@1.4.11)(webpack-cli@5.1.4): resolution: {integrity: sha512-rzVwlLeBWHJbmgTC/8TvAcu5vpJNII+MelQpylD4jNERPwpBJOE2lEcko1zJX3QJeLjTTAnQxn/OJ8bjDzVQaw==}