From 527e477d15441219216ed05b220cf8d48fa2d258 Mon Sep 17 00:00:00 2001 From: Niklas Mischkulnig <4586894+mischnic@users.noreply.github.com> Date: Sat, 3 Sep 2022 19:00:15 +0200 Subject: [PATCH] Verify version when resolving Node builtin polyfills (#8387) --- package.json | 1 + .../package-manager/src/NodePackageManager.js | 12 +- .../package-manager/src/NodeResolverBase.js | 20 +-- packages/core/utils/src/getModuleParts.js | 23 +++ packages/core/utils/src/index.js | 1 + .../resolvers/default/src/DefaultResolver.js | 5 +- .../node-resolver-core/src/NodeResolver.js | 163 ++++++++++-------- .../utils/node-resolver-core/src/builtins.js | 67 ++++--- .../utils/node-resolver-core/test/resolver.js | 4 +- 9 files changed, 172 insertions(+), 124 deletions(-) create mode 100644 packages/core/utils/src/getModuleParts.js diff --git a/package.json b/package.json index 762dd114e2d..d7c5370a3c6 100644 --- a/package.json +++ b/package.json @@ -54,6 +54,7 @@ "mocha-junit-reporter": "^2.0.0", "mocha-multi-reporters": "^1.5.1", "prettier": "2.4.1", + "punycode": "^1.4.1", "rimraf": "^3.0.2", "semver": "^5.7.1", "sinon": "^7.3.1" diff --git a/packages/core/package-manager/src/NodePackageManager.js b/packages/core/package-manager/src/NodePackageManager.js index 9e5d665f542..39e4f5f1380 100644 --- a/packages/core/package-manager/src/NodePackageManager.js +++ b/packages/core/package-manager/src/NodePackageManager.js @@ -22,6 +22,7 @@ import Module from 'module'; import path from 'path'; import semver from 'semver'; +import {getModuleParts} from '@parcel/utils'; import {getConflictingLocalDependencies} from './utils'; import {installPackage} from './installPackage'; import pkg from '../package.json'; @@ -138,7 +139,7 @@ export class NodePackageManager implements PackageManager { } async resolve( - name: DependencySpecifier, + id: DependencySpecifier, from: FilePath, options?: ?{| range?: ?SemverRange, @@ -147,11 +148,12 @@ export class NodePackageManager implements PackageManager { |}, ): Promise { let basedir = path.dirname(from); - let key = basedir + ':' + name; + let key = basedir + ':' + id; let resolved = cache.get(key); if (!resolved) { + let [name] = getModuleParts(id); try { - resolved = await this.resolver.resolve(name, from); + resolved = await this.resolver.resolve(id, from); } catch (e) { if ( e.code !== 'MODULE_NOT_FOUND' || @@ -189,7 +191,7 @@ export class NodePackageManager implements PackageManager { saveDev: options?.saveDev ?? true, }); - return this.resolve(name, from, { + return this.resolve(id, from, { ...options, shouldAutoInstall: false, }); @@ -230,7 +232,7 @@ export class NodePackageManager implements PackageManager { if (conflicts == null && options?.shouldAutoInstall === true) { await this.install([{name, range}], from); - return this.resolve(name, from, { + return this.resolve(id, from, { ...options, shouldAutoInstall: false, }); diff --git a/packages/core/package-manager/src/NodeResolverBase.js b/packages/core/package-manager/src/NodeResolverBase.js index ff4a1a0291c..e79c5b3f9bf 100644 --- a/packages/core/package-manager/src/NodeResolverBase.js +++ b/packages/core/package-manager/src/NodeResolverBase.js @@ -13,7 +13,7 @@ import type {ResolveResult} from './types'; import Module from 'module'; import path from 'path'; import invariant from 'assert'; -import {normalizeSeparators} from '@parcel/utils'; +import {getModuleParts} from '@parcel/utils'; const builtins = {pnpapi: true}; for (let builtin of Module.builtinModules) { @@ -102,22 +102,6 @@ export class NodeResolverBase { }); } - getModuleParts(name: string): [FilePath, ?string] { - name = path.normalize(name); - let splitOn = name.indexOf(path.sep); - if (name.charAt(0) === '@') { - splitOn = name.indexOf(path.sep, splitOn + 1); - } - if (splitOn < 0) { - return [normalizeSeparators(name), undefined]; - } else { - return [ - normalizeSeparators(name.substring(0, splitOn)), - name.substring(splitOn + 1) || undefined, - ]; - } - } - isBuiltin(name: DependencySpecifier): boolean { return !!(builtins[name] || name.startsWith('node:')); } @@ -135,7 +119,7 @@ export class NodeResolverBase { }; } - let [moduleName, subPath] = this.getModuleParts(id); + let [moduleName, subPath] = getModuleParts(id); let dir = path.dirname(sourceFile); let moduleDir = this.fs.findNodeModule(moduleName, dir); diff --git a/packages/core/utils/src/getModuleParts.js b/packages/core/utils/src/getModuleParts.js new file mode 100644 index 00000000000..41308ead1fd --- /dev/null +++ b/packages/core/utils/src/getModuleParts.js @@ -0,0 +1,23 @@ +// @flow strict-local +import path from 'path'; + +import {normalizeSeparators} from './path'; + +/** + * Returns the package name and the optional subpath + */ +export default function getModuleParts(_name: string): [string, ?string] { + let name = path.normalize(_name); + let splitOn = name.indexOf(path.sep); + if (name.charAt(0) === '@') { + splitOn = name.indexOf(path.sep, splitOn + 1); + } + if (splitOn < 0) { + return [normalizeSeparators(name), undefined]; + } else { + return [ + normalizeSeparators(name.substring(0, splitOn)), + name.substring(splitOn + 1) || undefined, + ]; + } +} diff --git a/packages/core/utils/src/index.js b/packages/core/utils/src/index.js index 2dbc483c826..63bf339e0ab 100644 --- a/packages/core/utils/src/index.js +++ b/packages/core/utils/src/index.js @@ -11,6 +11,7 @@ export {default as countLines} from './countLines'; export {default as generateBuildMetrics} from './generateBuildMetrics'; export {default as generateCertificate} from './generateCertificate'; export {default as getCertificate} from './getCertificate'; +export {default as getModuleParts} from './getModuleParts'; export {default as getRootDir} from './getRootDir'; export {default as isDirectoryInside} from './isDirectoryInside'; export {default as isURL} from './is-url'; diff --git a/packages/resolvers/default/src/DefaultResolver.js b/packages/resolvers/default/src/DefaultResolver.js index 835c881d4d1..6b166af04df 100644 --- a/packages/resolvers/default/src/DefaultResolver.js +++ b/packages/resolvers/default/src/DefaultResolver.js @@ -25,9 +25,8 @@ export default (new Resolver({ ? ['ts', 'tsx', 'js', 'jsx', 'json'] : [], mainFields: ['source', 'browser', 'module', 'main'], - packageManager: options.shouldAutoInstall - ? options.packageManager - : undefined, + packageManager: options.packageManager, + shouldAutoInstall: options.shouldAutoInstall, logger, }); diff --git a/packages/utils/node-resolver-core/src/NodeResolver.js b/packages/utils/node-resolver-core/src/NodeResolver.js index 9e89cd02177..51663daaddb 100644 --- a/packages/utils/node-resolver-core/src/NodeResolver.js +++ b/packages/utils/node-resolver-core/src/NodeResolver.js @@ -22,6 +22,7 @@ import { findAlternativeNodeModules, findAlternativeFiles, loadConfig, + getModuleParts, globToRegex, isGlobMatch, } from '@parcel/utils'; @@ -46,6 +47,7 @@ type Options = {| mainFields: Array, packageManager?: PackageManager, logger?: PluginLogger, + shouldAutoInstall?: boolean, |}; type ResolvedFile = {| path: string, @@ -96,6 +98,7 @@ export default class NodeResolver { packageCache: Map; rootPackage: InternalPackageJSON | null; packageManager: ?PackageManager; + shouldAutoInstall: boolean; logger: ?PluginLogger; constructor(opts: Options) { @@ -108,6 +111,7 @@ export default class NodeResolver { this.packageCache = new Map(); this.rootPackage = null; this.packageManager = opts.packageManager; + this.shouldAutoInstall = opts.shouldAutoInstall ?? false; this.logger = opts.logger; } @@ -271,10 +275,10 @@ export default class NodeResolver { let builtin = this.findBuiltin(filename, env); if (builtin === null) { return null; - } else if (builtin === empty) { + } else if (builtin && builtin.name === empty) { return {filePath: empty}; } else if (builtin !== undefined) { - filename = builtin; + filename = builtin.name; } if (this.shouldIncludeNodeModule(env, filename) === false) { @@ -287,51 +291,26 @@ export default class NodeResolver { // Resolve the module in node_modules let resolved: ?Module; try { - resolved = this.findNodeModulePath(filename, sourceFile, ctx); + resolved = this.findNodeModulePath( + filename, + !builtin ? sourceFile : path.join(this.projectRoot, 'index'), + ctx, + ); } catch (err) { // ignore } - // Auto install node builtin polyfills if not already available - if (resolved === undefined && builtin != null) { - let packageName = builtin.split('/')[0]; + // Autoinstall/verify version of builtin polyfills + if (builtin?.range != null) { + // This assumes that there are no polyfill packages that are scoped + // Append '/' to force this.packageManager to look up the package in node_modules + let packageName = builtin.name.split('/')[0] + '/'; let packageManager = this.packageManager; - if (packageManager) { - this.logger?.warn({ - message: md`Auto installing polyfill for Node builtin module "${specifier}"...`, - codeFrames: [ - { - filePath: ctx.loc?.filePath ?? sourceFile, - codeHighlights: ctx.loc - ? [ - { - message: 'used here', - start: ctx.loc.start, - end: ctx.loc.end, - }, - ] - : [], - }, - ], - documentationURL: - 'https://parceljs.org/features/node-emulation/#polyfilling-%26-excluding-builtin-node-modules', - }); - - await packageManager.resolve(builtin, this.projectRoot + '/index', { - saveDev: true, - shouldAutoInstall: true, - }); - - // Re-resolve - try { - resolved = this.findNodeModulePath(filename, sourceFile, ctx); - } catch (err) { - // ignore - } - } else { - throw new ThrowableDiagnostic({ - diagnostic: { - message: md`Node builtin polyfill "${packageName}" is not installed, but auto install is disabled.`, + if (resolved == null) { + // Auto install the Node builtin polyfills + if (this.shouldAutoInstall && packageManager) { + this.logger?.warn({ + message: md`Auto installing polyfill for Node builtin module "${specifier}"...`, codeFrames: [ { filePath: ctx.loc?.filePath ?? sourceFile, @@ -348,17 +327,74 @@ export default class NodeResolver { ], documentationURL: 'https://parceljs.org/features/node-emulation/#polyfilling-%26-excluding-builtin-node-modules', - hints: [ - md`Install the "${packageName}" package with your package manager, and run Parcel again.`, - ], + }); + + await packageManager.resolve( + packageName, + this.projectRoot + '/index', + { + saveDev: true, + shouldAutoInstall: true, + range: builtin.range, + }, + ); + + // Re-resolve + try { + resolved = this.findNodeModulePath( + filename, + this.projectRoot + '/index', + ctx, + ); + } catch (err) { + // ignore + } + } else { + throw new ThrowableDiagnostic({ + diagnostic: { + message: md`Node builtin polyfill "${packageName}" is not installed, but auto install is disabled.`, + codeFrames: [ + { + filePath: ctx.loc?.filePath ?? sourceFile, + codeHighlights: ctx.loc + ? [ + { + message: 'used here', + start: ctx.loc.start, + end: ctx.loc.end, + }, + ] + : [], + }, + ], + documentationURL: + 'https://parceljs.org/features/node-emulation/#polyfilling-%26-excluding-builtin-node-modules', + hints: [ + md`Install the "${packageName}" package with your package manager, and run Parcel again.`, + ], + }, + }); + } + } else if (builtin.range != null) { + // Assert correct version + + // TODO packageManager can be null for backwards compatibility, but that could cause invalid + // resolutions in monorepos + await packageManager?.resolve( + packageName, + this.projectRoot + '/index', + { + saveDev: true, + shouldAutoInstall: this.shouldAutoInstall, + range: builtin.range, }, - }); + ); } } if (resolved === undefined && process.versions.pnp != null && parent) { try { - let [moduleName, subPath] = this.getModuleParts(filename); + let [moduleName, subPath] = getModuleParts(filename); // $FlowFixMe[prop-missing] let pnp = _Module.findPnpApi(path.dirname(parent)); @@ -389,7 +425,7 @@ export default class NodeResolver { // If we couldn't resolve the node_modules path, just return the module name info if (resolved === undefined) { - let [moduleName, subPath] = this.getModuleParts(filename); + let [moduleName, subPath] = getModuleParts(filename); resolved = ({ moduleName, subPath, @@ -429,12 +465,12 @@ export default class NodeResolver { } if (Array.isArray(includeNodeModules)) { - let [moduleName] = this.getModuleParts(name); + let [moduleName] = getModuleParts(name); return includeNodeModules.includes(moduleName); } if (includeNodeModules && typeof includeNodeModules === 'object') { - let [moduleName] = this.getModuleParts(name); + let [moduleName] = getModuleParts(name); let include = includeNodeModules[moduleName]; if (include != null) { return !!include; @@ -447,7 +483,7 @@ export default class NodeResolver { name: string, ctx: ResolverContext, ) { - let [moduleName] = this.getModuleParts(name); + let [moduleName] = getModuleParts(name); let pkg = await this.findPackage(sourceFile, ctx); if (!pkg) { return; @@ -707,7 +743,10 @@ export default class NodeResolver { return resolvedFile; } - findBuiltin(filename: string, env: Environment): ?string { + findBuiltin( + filename: string, + env: Environment, + ): ?{|name: string, range: ?string|} { const isExplicitNode = filename.startsWith('node:'); if (isExplicitNode || builtins[filename]) { if (env.isNode()) { @@ -739,7 +778,7 @@ export default class NodeResolver { sourceFile: FilePath, ctx: ResolverContext, ): ?Module { - let [moduleName, subPath] = this.getModuleParts(filename); + let [moduleName, subPath] = getModuleParts(filename); ctx.invalidateOnFileCreate.push({ fileName: `node_modules/${moduleName}`, @@ -1150,7 +1189,7 @@ export default class NodeResolver { alias = this.lookupAlias(aliases, normalizeSeparators(filename)); if (alias == null) { // If it didn't match, try only the module name. - let [moduleName, subPath] = this.getModuleParts(filename); + let [moduleName, subPath] = getModuleParts(filename); alias = this.lookupAlias(aliases, moduleName); if (typeof alias === 'string' && subPath) { let isRelative = alias.startsWith('./'); @@ -1288,22 +1327,6 @@ export default class NodeResolver { return this.resolveAliases(filename, env, pkg); } - getModuleParts(name: string): [FilePath, ?string] { - name = path.normalize(name); - let splitOn = name.indexOf(path.sep); - if (name.charAt(0) === '@') { - splitOn = name.indexOf(path.sep, splitOn + 1); - } - if (splitOn < 0) { - return [normalizeSeparators(name), undefined]; - } else { - return [ - normalizeSeparators(name.substring(0, splitOn)), - name.substring(splitOn + 1) || undefined, - ]; - } - } - hasSideEffects(filePath: FilePath, pkg: InternalPackageJSON): boolean { switch (typeof pkg.sideEffects) { case 'boolean': diff --git a/packages/utils/node-resolver-core/src/builtins.js b/packages/utils/node-resolver-core/src/builtins.js index d733e527811..2abcafc6a3f 100644 --- a/packages/utils/node-resolver-core/src/builtins.js +++ b/packages/utils/node-resolver-core/src/builtins.js @@ -1,38 +1,53 @@ // @flow strict-local // $FlowFixMe this is untyped import {builtinModules} from 'module'; +import nullthrows from 'nullthrows'; +// flowlint-next-line untyped-import:off +import packageJson from '../package.json'; export const empty: string = require.resolve('./_empty.js'); -// $FlowFixMe -let builtins: {[string]: any, ...} = Object.create(null); +let builtins: {[string]: {|name: string, range: ?string|}, ...} = + // $FlowFixMe + Object.create(null); + // use definite (current) list of Node builtins for (let key of builtinModules) { - builtins[key] = empty; + builtins[key] = {name: empty, range: null}; } -builtins.assert = 'assert/'; -builtins.buffer = 'buffer/'; -builtins.console = 'console-browserify'; -builtins.constants = 'constants-browserify'; -builtins.crypto = 'crypto-browserify'; -builtins.domain = 'domain-browser'; -builtins.events = 'events/'; -builtins.http = 'stream-http'; -builtins.https = 'https-browserify'; -builtins.os = 'os-browserify/browser.js'; -builtins.path = 'path-browserify'; -builtins.process = 'process/browser.js'; -builtins.punycode = 'punycode/'; -builtins.querystring = 'querystring-es3/'; -builtins.stream = 'stream-browserify'; -builtins.string_decoder = 'string_decoder/'; -builtins.sys = 'util/util.js'; -builtins.timers = 'timers-browserify'; -builtins.tty = 'tty-browserify'; -builtins.url = 'url/'; -builtins.util = 'util/util.js'; -builtins.vm = 'vm-browserify'; -builtins.zlib = 'browserify-zlib'; +let polyfills = { + assert: 'assert', + buffer: 'buffer', + console: 'console-browserify', + constants: 'constants-browserify', + crypto: 'crypto-browserify', + domain: 'domain-browser', + events: 'events', + http: 'stream-http', + https: 'https-browserify', + os: 'os-browserify', + path: 'path-browserify', + process: 'process', + punycode: 'punycode', + querystring: 'querystring-es3', + stream: 'stream-browserify', + string_decoder: 'string_decoder', + sys: 'util', + timers: 'timers-browserify', + tty: 'tty-browserify', + url: 'url', + util: 'util', + vm: 'vm-browserify', + zlib: 'browserify-zlib', +}; + +for (let k in polyfills) { + let polyfill = polyfills[k]; + builtins[k] = { + name: polyfill + (builtinModules.includes(polyfill) ? '/' : ''), + range: nullthrows(packageJson.devDependencies[polyfill]), + }; +} export default builtins; diff --git a/packages/utils/node-resolver-core/test/resolver.js b/packages/utils/node-resolver-core/test/resolver.js index e28f9bf5f7f..9b130fc9fbe 100644 --- a/packages/utils/node-resolver-core/test/resolver.js +++ b/packages/utils/node-resolver-core/test/resolver.js @@ -305,7 +305,7 @@ describe('resolver', function () { }, { fileName: 'node_modules/browserify-zlib', - aboveFilePath: path.join(rootDir, 'foo.js'), + aboveFilePath: path.join(rootDir, 'index'), }, { fileName: 'package.json', @@ -341,7 +341,7 @@ describe('resolver', function () { }, { fileName: 'node_modules/browserify-zlib', - aboveFilePath: path.join(rootDir, 'foo.js'), + aboveFilePath: path.join(rootDir, 'index'), }, { fileName: 'package.json',