Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace fast-glob with fdir and picomatch, optimize peer dep validation crawler #952

Merged
merged 3 commits into from May 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions .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 dependencies in `node_modules`. This feature has been disabled for PNPM repos until further notice.
5 changes: 5 additions & 0 deletions .changeset/orange-laws-perform.md
@@ -0,0 +1,5 @@
---
'sku': patch
---

Replace `fast-glob` with `fdir` and `picomatch`
7 changes: 7 additions & 0 deletions .changeset/young-seals-shave.md
@@ -0,0 +1,7 @@
---
'sku': patch
---

Improve performance of peer dependency validation

Peer dependency validation shoould now complete within a few seconds, rather than a few minutes.
104 changes: 65 additions & 39 deletions packages/sku/context/defaultCompilePackages.js
@@ -1,57 +1,83 @@
const { posix: path } = require('node:path');
const chalk = require('chalk');
const glob = require('fast-glob');
const { fdir: Fdir } = require('fdir');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rename the import to prevent https://eslint.org/docs/latest/rules/new-cap at the call site.


const { cwd: skuCwd } = require('../lib/cwd');
const { cwd } = require('../lib/cwd');
const toPosixPath = require('../lib/toPosixPath');

const { rootDir, isPnpm } = require('../lib/packageManager');
const debug = require('debug')('sku:compilePackages');

/** @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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to how fdir handles symlinks, I had to special-case some code for pnpm, in addition to the pnpm-related block below.

// Follow symlinks inside node_modules into the pnpm virtual store
crawler = crawler.withSymlinks().withRelativePaths();
} else {
crawler = crawler.withBasePath();
}

globs.push(pnpmVirtualStoreGlob);
}
const seekDependencyGlob = '**/@seek/*/package.json';

let results = crawler
.glob(seekDependencyGlob)
.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(seekDependencyGlob)
.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);
}
}

debug(detectedCompilePackages);

module.exports = [
'sku',
'braid-design-system',
Expand Down
12 changes: 9 additions & 3 deletions 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');
Expand All @@ -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.`,
Expand Down
56 changes: 34 additions & 22 deletions 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');

/**
Expand All @@ -21,30 +21,42 @@ 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 {
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(),
},
/** @type {string[]} */
const duplicatePackages = [];
const packagesToCheck = [...paths.compilePackages, ...singletonPackages];

const packagePatterns = packagesToCheck.map((packageName) => [
packageName,
`node_modules/${packageName}/package.json`,
]);

const patterns = packagePatterns.map(([, pattern]) => pattern);

const results = await new Fdir()
.withBasePath()
.filter((file) => patterns.some((pattern) => file.endsWith(pattern)))
.crawl('./node_modules')
.withPromise();

for (const [packageName, pattern] of packagePatterns) {
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));

Expand All @@ -64,14 +76,14 @@ module.exports = async () => {
compile_package: packageName,
});
banner('error', 'Error: Duplicate packages detected', messages);
}

packages.push(...results);
duplicatePackages.push(...resultsForPackage);
}
}

const compilePackages = new Map();

await asyncMap(packages, async (p) => {
await asyncMap(duplicatePackages, async (p) => {
const contents = await readFile(getPathFromCwd(p), {
encoding: 'utf8',
});
Expand Down
4 changes: 3 additions & 1 deletion packages/sku/package.json
Expand Up @@ -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",
Expand All @@ -104,6 +104,7 @@
"node-html-parser": "^6.1.1",
"open": "^7.3.1",
"path-to-regexp": "^6.2.0",
"picomatch": "^3.0.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

picomatch v4 is the latest, but fdir's peer dependency hasn't been updated to support it. We're not really missing anything by staying on v3 for now.

"postcss": "^8.4.0",
"postcss-loader": "^8.0.0",
"prettier": "^2.8.8",
Expand Down Expand Up @@ -132,6 +133,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",
Expand Down
10 changes: 6 additions & 4 deletions packages/sku/scripts/init.js
Expand Up @@ -20,7 +20,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');
Expand Down Expand Up @@ -151,9 +151,11 @@ const packageNameRegex =
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,
Expand Down
32 changes: 29 additions & 3 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.