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.

Sign up for GitHub

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

Jump to bottom

Uses NODE_OPTIONS with PnP instead of CLI args #6629

Merged
merged 4 commits into from Nov 5, 2018
Merged

Uses NODE_OPTIONS with PnP instead of CLI args #6629

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fcb9cfc
Uses NODE_OPTIONS when spawning a process
Nov 3, 2018
8ab4cea
Adds an additional test
Nov 5, 2018
21c5092
Update CHANGELOG.md
arcanis Nov 5, 2018
ced838b
Merge branch 'master' into node-options
arcanis Nov 5, 2018
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
53 changes: 53 additions & 0 deletions packages/pkg-tests/pkg-tests-specs/sources/pnp.js
View file
Open in desktop
Expand Up @@ -1340,5 +1340,58 @@ module.exports = makeTemporaryEnv => {
},
),
);

test(
`it should not break spawning new Node processes ('node' command)`,
makeTemporaryEnv(
{
dependencies: {[`no-deps`]: `1.0.0`},
},
{plugNPlay: true},
async ({path, run, source}) => {
await run(`install`);

await writeFile(`${path}/script.js`, `console.log(JSON.stringify(require('no-deps')))`);

await expect(
source(
`JSON.parse(require('child_process').execFileSync(process.execPath, [${JSON.stringify(
`${path}/script.js`,
)}]).toString())`,
),
).resolves.toMatchObject({
name: `no-deps`,
version: `1.0.0`,
});
},
),
);

test(
`it should not break spawning new Node processes ('run' command)`,
makeTemporaryEnv(
{
dependencies: {[`no-deps`]: `1.0.0`},
scripts: {[`script`]: `node main.js`},
},
{plugNPlay: true},
async ({path, run, source}) => {
await run(`install`);

await writeFile(`${path}/sub.js`, `console.log(JSON.stringify(require('no-deps')))`);
await writeFile(
`${path}/main.js`,
`console.log(require('child_process').execFileSync(process.execPath, [${JSON.stringify(
`${path}/sub.js`,
)}]).toString())`,
);

expect(JSON.parse((await run(`run`, `script`)).stdout)).toMatchObject({
name: `no-deps`,
version: `1.0.0`,
});
},
),
);
Copy link
Member

Choose a reason for hiding this comment

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

I think we need two more tests:

  1. For cases where we spawn another script and purposefully omit node args which simulates the scenario you are solving for (is this the second test, I can't really tell?)
  2. Have an already set NODE_OPTIONS env variable and make sure it is preserved.

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 first test tests yarn node my-script.js', and the second tests yarn run my-script. I think that should cover the first case? Will add a test for empty NODE_OPTIONS 👍

});
};
4 changes: 3 additions & 1 deletion src/cli/commands/node.js
View file
Open in desktop
Expand Up @@ -21,14 +21,16 @@ export function hasWrapper(commander: Object, args: Array<string>): boolean {
export async function run(config: Config, reporter: Reporter, flags: Object, args: Array<string>): Promise<void> {
const pnpPath = `${config.lockfileFolder}/${PNP_FILENAME}`;

let nodeOptions = process.env.NODE_OPTIONS || '';
if (await fs.exists(pnpPath)) {
args = ['-r', pnpPath, ...args];
nodeOptions += ` --require ${pnpPath}`;
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing the leading space wouldn't be an issue when NODE_OPTIONS is empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, will work 👍

Copy link
Contributor

@jdalton jdalton Jan 20, 2019 •
edited

Choose a reason for hiding this comment

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

👆 It looks like

nodeOptions += ` --require ${pnpPath}`

should be

nodeOptions = `--require ${pnpPath} ${nodeOptions}`

so that pnp is the first preloaded module.

Update:

Issue filed #6941; PR #6942.

}

try {
await child.spawn(NODE_BIN_PATH, args, {
stdio: 'inherit',
cwd: flags.into || config.cwd,
env: {...process.env, NODE_OPTIONS: nodeOptions},
});
} catch (err) {
throw err;
Expand Down
19 changes: 8 additions & 11 deletions src/util/execute-lifecycle-script.js
View file
Open in desktop
Expand Up @@ -26,14 +26,6 @@ export const IGNORE_MANIFEST_KEYS: Set<string> = new Set(['readme', 'notice', 'l
// See https://github.com/yarnpkg/yarn/issues/2286.
const IGNORE_CONFIG_KEYS = ['lastUpdateCheck'];

async function getPnpParameters(config: Config): Promise<Array<string>> {
if (await fs.exists(`${config.lockfileFolder}/${constants.PNP_FILENAME}`)) {
return ['-r', `${config.lockfileFolder}/${constants.PNP_FILENAME}`];
} else {
return [];
}
}

let wrappersFolder = null;

export async function getWrappersFolder(config: Config): Promise<string> {
Expand All @@ -45,7 +37,6 @@ export async function getWrappersFolder(config: Config): Promise<string> {

await makePortableProxyScript(process.execPath, wrappersFolder, {
Copy link
Member

Choose a reason for hiding this comment

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

Shall we unify this logic in the future and use node at all times?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean? At most we should replace makePortableScript with cmdShim (which already covers the behavior we're using here, I think)

proxyBasename: 'node',
prependArguments: [...(await getPnpParameters(config))],
});

await makePortableProxyScript(process.execPath, wrappersFolder, {
Expand Down Expand Up @@ -212,8 +203,9 @@ export async function makeEnv(
}
}

if (await fs.exists(`${config.lockfileFolder}/${constants.PNP_FILENAME}`)) {
const pnpApi = dynamicRequire(`${config.lockfileFolder}/${constants.PNP_FILENAME}`);
const pnpFile = `${config.lockfileFolder}/${constants.PNP_FILENAME}`;
if (await fs.exists(pnpFile)) {
const pnpApi = dynamicRequire(pnpFile);

const packageLocator = pnpApi.findPackageLocator(`${config.cwd}/`);
const packageInformation = pnpApi.getPackageInformation(packageLocator);
Expand All @@ -227,6 +219,11 @@ export async function makeEnv(

pathParts.unshift(`${dependencyInformation.packageLocation}/.bin`);
}

// Note that NODE_OPTIONS doesn't support any style of quoting its arguments at the moment
Copy link
Member

Choose a reason for hiding this comment

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

Referring to your PR on Node would be nice. Also, would be great if we can centralize some of this PnP path logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'll keep it duplicated for now - since it's very few lines, I'm concerned about over-abstracting it (since it's only used in two places and there are no reasons to think it will change)

// For this reason, it won't work if the user has a space inside its $PATH
env.NODE_OPTIONS = env.NODE_OPTIONS || '';
env.NODE_OPTIONS += ` --require ${pnpFile}`;
}

pathParts.unshift(await getWrappersFolder(config));
Expand Down