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

Verify version when resolving Node builtin polyfills #8387

Merged
merged 9 commits into from Sep 3, 2022
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
1 change: 1 addition & 0 deletions package.json
Expand Up @@ -54,6 +54,7 @@
"mocha-junit-reporter": "^2.0.0",
"mocha-multi-reporters": "^1.5.1",
"prettier": "2.4.1",
"punycode": "^1.4.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

The url polyfill package declares punycode as a dependency

https://github.com/defunctzombie/node-url/blob/5480ec001102457ac00c3c7878facdec39b536c8/package.json#L6-L9

but then does require("punycode") which uses the node polyfill (as opposed to require("punycode/")). I needed to declare this version to get the integration tests to pass....

"rimraf": "^3.0.2",
"semver": "^5.7.1",
"sinon": "^7.3.1"
Expand Down
12 changes: 7 additions & 5 deletions packages/core/package-manager/src/NodePackageManager.js
Expand Up @@ -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';
Expand Down Expand Up @@ -138,7 +139,7 @@ export class NodePackageManager implements PackageManager {
}

async resolve(
name: DependencySpecifier,
id: DependencySpecifier,
from: FilePath,
options?: ?{|
range?: ?SemverRange,
Expand All @@ -147,11 +148,12 @@ export class NodePackageManager implements PackageManager {
|},
): Promise<ResolveResult> {
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' ||
Expand Down Expand Up @@ -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,
});
Expand Down Expand Up @@ -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,
});
Expand Down
20 changes: 2 additions & 18 deletions packages/core/package-manager/src/NodeResolverBase.js
Expand Up @@ -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) {
Expand Down Expand Up @@ -102,22 +102,6 @@ export class NodeResolverBase<T> {
});
}

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:'));
}
Expand All @@ -135,7 +119,7 @@ export class NodeResolverBase<T> {
};
}

let [moduleName, subPath] = this.getModuleParts(id);
let [moduleName, subPath] = getModuleParts(id);
let dir = path.dirname(sourceFile);
let moduleDir = this.fs.findNodeModule(moduleName, dir);

Expand Down
23 changes: 23 additions & 0 deletions 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,
];
}
}
1 change: 1 addition & 0 deletions packages/core/utils/src/index.js
Expand Up @@ -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';
Expand Down
5 changes: 2 additions & 3 deletions packages/resolvers/default/src/DefaultResolver.js
Expand Up @@ -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,
});

Expand Down