From 06e2b960ede3406b7f2da30d0cc8a47236e9d70f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Mon, 22 Oct 2018 11:39:38 +0100 Subject: [PATCH 1/4] Update generate-pnp-map-api.tpl.js --- src/util/generate-pnp-map-api.tpl.js | 41 +++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/src/util/generate-pnp-map-api.tpl.js b/src/util/generate-pnp-map-api.tpl.js index f594353b07..dca837e40b 100644 --- a/src/util/generate-pnp-map-api.tpl.js +++ b/src/util/generate-pnp-map-api.tpl.js @@ -646,12 +646,45 @@ exports.setup = function setup() { if (!enableNativeHooks) { return originalModuleResolveFilename.call(Module, request, parent, isMain, options); } + + let issuers; + + if (options) { + const optionNames = new Set(Object.keys(optionNames)); + optionNames.delete('paths'); + + if (options.size > 0) { + throw makeError(`UNSUPPORTED`, `Some options passed to require() aren't supported by PnP yet (${Array.from(optionNames).join(', ')})`); + } + + if (options.paths) { + issuers = options.paths.map(entry => `${path.normalize(entry)}/`); + } + } + + if (!issuers) { + const issuerModule = getIssuerModule(parent); + const issuer = issuerModule ? issuerModule.filename : `${process.cwd()}/`; + + issuers = [issuer]; + } + + let firstError; - const issuerModule = getIssuerModule(parent); - const issuer = issuerModule ? issuerModule.filename : process.cwd() + '/'; + for (const issuer of issuers) { + let resolution; + + try { + resolution = exports.resolveRequest(request, issuer); + } catch (error) { + firstError = firstError || error; + continue; + } - const resolution = exports.resolveRequest(request, issuer); - return resolution !== null ? resolution : request; + return resolution !== null ? resolution : request; + } + + throw firstError; }; const originalFindPath = Module._findPath; From f520df3450bc0aff1eeed215e28122fce1463523 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Mon, 22 Oct 2018 13:28:39 +0100 Subject: [PATCH 2/4] Updates the changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f63236285e..c846966611 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,10 @@ Please add one entry in this file for each change in Yarn's behavior. Use the sa [#6562](https://github.com/yarnpkg/yarn/pull/6562) - [**Bertrand Marron**](https://github.com/tusbar) +- Fixes `require.resolve` when used together with the `paths` option + + [#6565](https://github.com/yarnpkg/yarn/pull/6565) - [**Maël Nison**](https://twitter.com/arcanis) + ## 1.12.0 - Adds initial support for PnP on Windows From b3873867f9a187f8281e2825b7a6fbfa6d68ebff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Mon, 22 Oct 2018 14:11:51 +0100 Subject: [PATCH 3/4] Adds tests --- .../pkg-tests/pkg-tests-specs/sources/pnp.js | 102 ++++++++++++++++++ src/util/generate-pnp-map-api.tpl.js | 23 ++-- 2 files changed, 115 insertions(+), 10 deletions(-) diff --git a/packages/pkg-tests/pkg-tests-specs/sources/pnp.js b/packages/pkg-tests/pkg-tests-specs/sources/pnp.js index 3c70197991..9afa12fe06 100644 --- a/packages/pkg-tests/pkg-tests-specs/sources/pnp.js +++ b/packages/pkg-tests/pkg-tests-specs/sources/pnp.js @@ -460,6 +460,108 @@ module.exports = makeTemporaryEnv => { ), ); + test( + `it should support the 'paths' option from require.resolve (same dependency tree)`, + makeTemporaryEnv( + { + private: true, + workspaces: [`workspace-*`], + }, + { + plugNPlay: true, + }, + async ({path, run, source}) => { + await writeJson(`${path}/workspace-a/package.json`, { + name: `workspace-a`, + version: `1.0.0`, + dependencies: {[`no-deps`]: `1.0.0`}, + }); + + await writeJson(`${path}/workspace-b/package.json`, { + name: `workspace-b`, + version: `1.0.0`, + dependencies: {[`no-deps`]: `2.0.0`, [`one-fixed-dep`]: `1.0.0`}, + }); + + await run(`install`); + + await expect( + source( + `require(require.resolve('no-deps', {paths: ${JSON.stringify([ + `${path}/workspace-a`, + `${path}/workspace-b`, + ])}}))`, + ), + ).resolves.toMatchObject({ + name: `no-deps`, + version: `1.0.0`, + }); + }, + ), + ); + + // Skipped because not supported (we can't require files from within other dependency trees, since we couldn't + // reconcile them together: dependency tree A could think that package X has deps Y@1 while dependency tree B + // could think that X has deps Y@2 instead. Since they would share the same location on the disk, PnP wouldn't + // be able to tell which one should be used) + test.skip( + `it should support the 'paths' option from require.resolve (different dependency trees)`, + makeTemporaryEnv( + { + dependencies: {}, + }, + { + plugNPlay: true, + }, + async ({path, run, source}) => { + await run(`install`); + + const tmpA = await createTemporaryFolder(); + const tmpB = await createTemporaryFolder(); + + await writeJson(`${tmpA}/package.json`, { + dependencies: {[`no-deps`]: `1.0.0`}, + }); + + await writeJson(`${tmpB}/package.json`, { + dependencies: {[`no-deps`]: `2.0.0`, [`one-fixed-dep`]: `1.0.0`}, + }); + + await run(`install`, { + cwd: tmpA, + }); + + await run(`install`, { + cwd: tmpB, + }); + + await expect( + source(`require(require.resolve('no-deps', {paths: ${JSON.stringify([tmpA, tmpB])}}))`), + ).resolves.toMatchObject({ + name: `no-deps`, + version: `1.0.0`, + }); + }, + ), + ); + + test( + `using require.resolve with unsupported options should throw`, + makeTemporaryEnv( + { + dependencies: {[`no-deps`]: `1.0.0`}, + }, + { + plugNPlay: true, + }, + async ({path, run, source}) => { + await run(`install`); + + await expect(source(`require.resolve('no-deps', {foobar: 42})`)).rejects.toBeTruthy(); + }, + ), + ); + test( `it should load the index.js file when loading from a folder`, makeTemporaryEnv({}, {plugNPlay: true}, async ({path, run, source}) => { diff --git a/src/util/generate-pnp-map-api.tpl.js b/src/util/generate-pnp-map-api.tpl.js index dca837e40b..693b82394a 100644 --- a/src/util/generate-pnp-map-api.tpl.js +++ b/src/util/generate-pnp-map-api.tpl.js @@ -646,15 +646,18 @@ exports.setup = function setup() { if (!enableNativeHooks) { return originalModuleResolveFilename.call(Module, request, parent, isMain, options); } - + let issuers; - + if (options) { - const optionNames = new Set(Object.keys(optionNames)); + const optionNames = new Set(Object.keys(options)); optionNames.delete('paths'); - - if (options.size > 0) { - throw makeError(`UNSUPPORTED`, `Some options passed to require() aren't supported by PnP yet (${Array.from(optionNames).join(', ')})`); + + if (optionNames.size > 0) { + throw makeError( + `UNSUPPORTED`, + `Some options passed to require() aren't supported by PnP yet (${Array.from(optionNames).join(', ')})`, + ); } if (options.paths) { @@ -665,15 +668,15 @@ exports.setup = function setup() { if (!issuers) { const issuerModule = getIssuerModule(parent); const issuer = issuerModule ? issuerModule.filename : `${process.cwd()}/`; - + issuers = [issuer]; } - + let firstError; for (const issuer of issuers) { let resolution; - + try { resolution = exports.resolveRequest(request, issuer); } catch (error) { @@ -683,7 +686,7 @@ exports.setup = function setup() { return resolution !== null ? resolution : request; } - + throw firstError; }; From 75718ee274cc1636e88b7ecb8bd19e4aff46f7b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Tue, 23 Oct 2018 11:13:47 +0100 Subject: [PATCH 4/4] Scopes the option tests to >=8.9.0 --- .../pkg-tests/pkg-tests-specs/package.json | 3 +- .../pkg-tests/pkg-tests-specs/sources/pnp.js | 199 +++++++++--------- packages/pkg-tests/yarn.lock | 5 + 3 files changed, 108 insertions(+), 99 deletions(-) diff --git a/packages/pkg-tests/pkg-tests-specs/package.json b/packages/pkg-tests/pkg-tests-specs/package.json index 5ef9b6ab96..5b84ce264f 100644 --- a/packages/pkg-tests/pkg-tests-specs/package.json +++ b/packages/pkg-tests/pkg-tests-specs/package.json @@ -4,6 +4,7 @@ "main": "./sources/index.js", "dependencies": { "fs-extra": "^7.0.0", - "pkg-tests-core": "1.0.0" + "pkg-tests-core": "1.0.0", + "semver": "^5.6.0" } } diff --git a/packages/pkg-tests/pkg-tests-specs/sources/pnp.js b/packages/pkg-tests/pkg-tests-specs/sources/pnp.js index 9afa12fe06..ec95577320 100644 --- a/packages/pkg-tests/pkg-tests-specs/sources/pnp.js +++ b/packages/pkg-tests/pkg-tests-specs/sources/pnp.js @@ -1,6 +1,7 @@ const cp = require('child_process'); const {existsSync, statSync, stat, rename, readdir, remove} = require('fs-extra'); const {relative, isAbsolute} = require('path'); +const {satisfies} = require('semver'); const { fs: {createTemporaryFolder, readFile, readJson, writeFile, writeJson}, @@ -460,107 +461,109 @@ module.exports = makeTemporaryEnv => { ), ); - test( - `it should support the 'paths' option from require.resolve (same dependency tree)`, - makeTemporaryEnv( - { - private: true, - workspaces: [`workspace-*`], - }, - { - plugNPlay: true, - }, - async ({path, run, source}) => { - await writeJson(`${path}/workspace-a/package.json`, { - name: `workspace-a`, - version: `1.0.0`, - dependencies: {[`no-deps`]: `1.0.0`}, - }); - - await writeJson(`${path}/workspace-b/package.json`, { - name: `workspace-b`, - version: `1.0.0`, - dependencies: {[`no-deps`]: `2.0.0`, [`one-fixed-dep`]: `1.0.0`}, - }); - - await run(`install`); - - await expect( - source( - `require(require.resolve('no-deps', {paths: ${JSON.stringify([ - `${path}/workspace-a`, - `${path}/workspace-b`, - ])}}))`, - ), - ).resolves.toMatchObject({ - name: `no-deps`, - version: `1.0.0`, - }); - }, - ), - ); - - // Skipped because not supported (we can't require files from within other dependency trees, since we couldn't - // reconcile them together: dependency tree A could think that package X has deps Y@1 while dependency tree B - // could think that X has deps Y@2 instead. Since they would share the same location on the disk, PnP wouldn't - // be able to tell which one should be used) - test.skip( - `it should support the 'paths' option from require.resolve (different dependency trees)`, - makeTemporaryEnv( - { - dependencies: {}, - }, - { - plugNPlay: true, - }, - async ({path, run, source}) => { - await run(`install`); - - const tmpA = await createTemporaryFolder(); - const tmpB = await createTemporaryFolder(); + if (satisfies(process.versions.node, `>=8.9.0`)) { + test( + `it should support the 'paths' option from require.resolve (same dependency tree)`, + makeTemporaryEnv( + { + private: true, + workspaces: [`workspace-*`], + }, + { + plugNPlay: true, + }, + async ({path, run, source}) => { + await writeJson(`${path}/workspace-a/package.json`, { + name: `workspace-a`, + version: `1.0.0`, + dependencies: {[`no-deps`]: `1.0.0`}, + }); + + await writeJson(`${path}/workspace-b/package.json`, { + name: `workspace-b`, + version: `1.0.0`, + dependencies: {[`no-deps`]: `2.0.0`, [`one-fixed-dep`]: `1.0.0`}, + }); + + await run(`install`); + + await expect( + source( + `require(require.resolve('no-deps', {paths: ${JSON.stringify([ + `${path}/workspace-a`, + `${path}/workspace-b`, + ])}}))`, + ), + ).resolves.toMatchObject({ + name: `no-deps`, + version: `1.0.0`, + }); + }, + ), + ); + + // Skipped because not supported (we can't require files from within other dependency trees, since we couldn't + // reconcile them together: dependency tree A could think that package X has deps Y@1 while dependency tree B + // could think that X has deps Y@2 instead. Since they would share the same location on the disk, PnP wouldn't + // be able to tell which one should be used) + test.skip( + `it should support the 'paths' option from require.resolve (different dependency trees)`, + makeTemporaryEnv( + { + dependencies: {}, + }, + { + plugNPlay: true, + }, + async ({path, run, source}) => { + await run(`install`); + + const tmpA = await createTemporaryFolder(); + const tmpB = await createTemporaryFolder(); + + await writeJson(`${tmpA}/package.json`, { + dependencies: {[`no-deps`]: `1.0.0`}, + }); + + await writeJson(`${tmpB}/package.json`, { + dependencies: {[`no-deps`]: `2.0.0`, [`one-fixed-dep`]: `1.0.0`}, + }); + + await run(`install`, { + cwd: tmpA, + }); + + await run(`install`, { + cwd: tmpB, + }); + + await expect( + source(`require(require.resolve('no-deps', {paths: ${JSON.stringify([tmpA, tmpB])}}))`), + ).resolves.toMatchObject({ + name: `no-deps`, + version: `1.0.0`, + }); + }, + ), + ); - await writeJson(`${tmpA}/package.json`, { + test( + `using require.resolve with unsupported options should throw`, + makeTemporaryEnv( + { dependencies: {[`no-deps`]: `1.0.0`}, - }); - - await writeJson(`${tmpB}/package.json`, { - dependencies: {[`no-deps`]: `2.0.0`, [`one-fixed-dep`]: `1.0.0`}, - }); - - await run(`install`, { - cwd: tmpA, - }); - - await run(`install`, { - cwd: tmpB, - }); - - await expect( - source(`require(require.resolve('no-deps', {paths: ${JSON.stringify([tmpA, tmpB])}}))`), - ).resolves.toMatchObject({ - name: `no-deps`, - version: `1.0.0`, - }); - }, - ), - ); - - test( - `using require.resolve with unsupported options should throw`, - makeTemporaryEnv( - { - dependencies: {[`no-deps`]: `1.0.0`}, - }, - { - plugNPlay: true, - }, - async ({path, run, source}) => { - await run(`install`); + }, + { + plugNPlay: true, + }, + async ({path, run, source}) => { + await run(`install`); - await expect(source(`require.resolve('no-deps', {foobar: 42})`)).rejects.toBeTruthy(); - }, - ), - ); + await expect(source(`require.resolve('no-deps', {foobar: 42})`)).rejects.toBeTruthy(); + }, + ), + ); + } test( `it should load the index.js file when loading from a folder`, diff --git a/packages/pkg-tests/yarn.lock b/packages/pkg-tests/yarn.lock index 8adeb97797..c6cc754672 100644 --- a/packages/pkg-tests/yarn.lock +++ b/packages/pkg-tests/yarn.lock @@ -3150,6 +3150,11 @@ sax@^1.2.4: version "5.5.0" resolved "https://registry.yarnpkg.com/semver/-/semver-5.5.0.tgz#dc4bbc7a6ca9d916dee5d43516f0092b58f7b8ab" +semver@^5.6.0: + version "5.6.0" + resolved "https://registry.yarnpkg.com/semver/-/semver-5.6.0.tgz#7e74256fbaa49c75aa7c7a205cc22799cac80004" + integrity sha512-RS9R6R35NYgQn++fkDWaOmqGoj4Ek9gGs+DPxNUZKuwE183xjJroKvyo1IzVFeXvUrvmALy6FWD5xrdJT25gMg== + set-blocking@^2.0.0, set-blocking@~2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/set-blocking/-/set-blocking-2.0.0.tgz#045f9782d011ae9a6803ddd382b24392b3d890f7"