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

Add support for Yarn PnP #49

Closed
wants to merge 1 commit into from
Closed

Add support for Yarn PnP #49

wants to merge 1 commit into from

Conversation

Turbo87
Copy link

@Turbo87 Turbo87 commented Jan 18, 2019

Yarn Plug'n'Play is using a custom resolver to lookup dependencies from a machine-wide global location, instead of keeping node_modules duplicated in every project.

When yarn run is used and it detects that it is running in PnP mode it will call the child process using --require .pnp.js, with .pnp.js being a file generated per-project by Yarn. This will enable PnP mode for the child process and make the PnP API available via require('pnpapi').

Since workerpool is also forking off child processes it would need a similar mechanism to ensure that those child processes are able to resolve their dependencies correctly. This PR adds the necessary --require argument to the fork option if PnP mode is detected by checking process.versions.pnp.

In case you're wondering: we're working on making ember-cli compatible with Yarn PnP and that is using broccoli-babel-transpiler, which is using workerpool under the hood.

/cc @arcanis @rwjblue

@rwjblue
Copy link

rwjblue commented Jan 18, 2019

Chatted with @Turbo87 in DM, I think we should move to inheriting NODE_OPTIONS instead (I’m somewhat surprised we aren’t already inheriting all of process.env).

This change would essentially be following the same move in Yarn itself (away from using —require to specifying NODE_OPTIONS) in yarnpkg/yarn#6629.

@rwjblue
Copy link

rwjblue commented Jan 18, 2019

Ah, apparently child_process.fork does not automatically inherit the current processes process.env ( child_process.spawn has this behavior). That explains why it doesn’t “just work”.

See here for the two implementations (on Node 10):

So the question here is, should we inherit all of process.env or only process.env.NODE_OPTIONS?

@Turbo87
Copy link
Author

Turbo87 commented Jan 18, 2019

@rwjblue unfortunately, it's not quite as easy. fork() uses spawn() under the hood and that correctly forwards the existing env vars. but in our case when Yarn PnP mode is enabled via ember instead of using yarn run it will not have a NODE_OPTIONS env var with the correct value.

@Turbo87
Copy link
Author

Turbo87 commented Jan 18, 2019

we will most likely resolve this another way (adjusting NODE_OPTIONS ourselves once we detect Yarn PnP mode) so I'll close this PR for now. sorry for the noise 😉

@Turbo87 Turbo87 closed this Jan 18, 2019
@josdejong
Copy link
Owner

👍 thanks anyway Tobias for taking the time to put together a PR :)

@Turbo87 Turbo87 deleted the pnp branch January 19, 2019 17:56
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.

None yet

3 participants