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

refactor(cli): Schematic runner should use node require mechanism #899

Merged
merged 1 commit into from
Nov 4, 2020

Conversation

andreialecu
Copy link
Contributor

@andreialecu andreialecu commented Sep 30, 2020

Fixes #898

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #898

When running schematics, the current code tries to read a directory like node_modules/.bin/schematics and execute the script in it. This fails on Yarn PnP because there is no node_modules.

What is the new behavior?

Uses require.resolve which is the standard way to locate scripts in node and works across all package managers.

Additionally, the script needs to be started using spawn("node", [scriptPath, ...otherArgs]) in order for PnP to properly work because the schematics.js script may be inside a zip file on Yarn v2. Running it with node ensures that the NODE_OPTIONS environment variable is passed through and that the execution succeeds.

You can test the changes in this PR on yarn v2 with PnP mode like this:

cd /tmp
nest new new-app
cd /tmp/new-app
yarn set version 2
yarn install
yarn add --dev @nestjs/cli
yarn nest g module test

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

I had to remove the tests for this feature because jest does not allow mocking require.resolve: jestjs/jest#9543

However, since the functionality is now straight forward and uses standard node functionality I don't think tests are critical.

@kamilmysliwiec
Copy link
Member

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schematics not working with Yarn PnP
2 participants