Plug'n'Play support by arcanis · Pull Request #5136 · react/create-react-app

arcanis · 2018-09-27T14:54:18Z

Fixes#5117 - there's still at least one issue with ESLint which cannot load its plugins, and as a result crashes (eslint/eslint#10125), but at least it gets to this point 🙂

Timer · 2018-09-27T14:59:58Z

We probably need the Jest resolver too, right?

arcanis · 2018-09-27T15:05:37Z

Oh likely, yes - I tested yarn start & yarn build but not yarn test - let me check. I'll also split the process args forwarding in a separate PR, so that this one can stay open until everything works 👍

arcanis · 2018-09-27T15:23:17Z

Got it working with the plugin, just one interesting problem - Babel transforms the code to use the @babel/runtime package, which is nice and all. Problem is: @babel/runtime is not listed as dependency of the package containing the transformed code! So the resolution is ambiguous and fails 😐

If @babel/runtime was declared as dependency of the top-level, it would work without issue, because PnP fallbacks to the top-level for compatibility purposes. Unfortunately, in react-scripts's case, @babel/runtime isn't top-level, so it cannot be picked up by the fallback 😞

Apart from moving @babel/runtime to the top level, the only other possible fix I see would be for whatever transpile using @babel/runtime to use the full path (/tmp/cache/@babel-runtime-1.0.0/...) instead of the bare import (@babel/runtime/...). Will check if that can be done.

Timer · 2018-09-27T15:25:16Z

The full path option sounds great, especially because it's immediately bundled in other parts of the app.

arcanis · 2018-09-27T16:51:34Z

Pushed a commit that does exactly this (I'll move it in a separate PR later too so it can be merged independently) 👍

I also need to merge yarnpkg/yarn#6444 and yarnpkg/yarn#6443 on Yarn's side. Will then continue investigating to make sure everything's ok. Do you have suggestions to add tests? Can I just add a travis config that uses PnP to make the install?

Timer · 2018-09-27T16:57:58Z

We need to majorly overhaul our testing, but a travis config that uses PnP would be best for now.

…er PnP

arcanis · 2018-09-29T12:41:10Z

Here we go! All green 😃

Note that this PR only makes create-native-app compatible with PnP environments, but doesn't expose a way to toggle it on from the command line (apart from using yarn --pnp once the project has been created). Do you want me to open another PR for this?

gaearon · 2018-09-29T15:22:03Z

@arcanis Could you clarify what happens to ESLint in the meantime?

gaearon · 2018-09-29T15:22:46Z

Do you want me to open another PR for this?

You can add a --pnp flag to CRA in this PR. In fact I thought we already pass unknown flags to npm/Yarn. If we don't maybe we should just do that?

gaearon · 2018-09-29T15:23:07Z

+
+ const nodeArgs = fs.existsSync(pnpPath) ? ['--require', pnpPath] : [];
+
+ await executeNodeScript(


Does this work on Node LTS version?

I think so, it doesn't use any recent feature and the Node 8 tests pass.

gaearon · 2018-09-29T15:23:52Z

 ],
+ // Plug'n'Play relies on symlink for its virtual paths (ie peer dependencies), which Webpack
+ // always resolve to the absolute path on disk by default.
+ symlinks: false,


If we're not in PnP mode, would that create problems? It seems like potential breaking change, no?
I'm not sure how this impacts behavior but I wouldn't be surprised if it does.
Is this something we want to do anyway?

If we do this, symlinking src/ to node_modules/@ won't work because we'll treat app code as node_module code (instead of application code).
We could add special behavior to compile code from @ as app code, but I'd want to collect feedback on what people are using symlinks for currently -- I suspect it'll be in line with the behavior we want.

@Timer where is defined this @ symlink? Shouldn't it be an alias entry in webpack/jest?

We didn’t add this feature yet. We were hoping we could do it via symlink instead of adding a special alias to every tool. It would be nice to not confuse IDEs, Flow, etc, which symlink could help with.

arcanis · 2018-09-30T08:24:50Z

@arcanis Could you clarify what happens to ESLint in the meantime?

I added CRA/ESLint compatibility with yarnpkg/yarn#6449. It basically makes react-scripts a valid fallback for resolving dependencies. It's only a temporary measure since it removes some safety, but better than not working at all.

You can add a --pnp flag to CRA in this PR. In fact I thought we already pass unknown flags to npm/Yarn. If we don't maybe we should just do that?

Will check 👍

gaearon · 2018-09-30T12:59:51Z

symlinks: false worries me.

Can you describe effects of this change in detail, unrelated to PnP? I think this will probably break some existing use cases but I’m not sure they’re valid in the first place. I’m also not sure how this relates to Jest behavior (does Jest match behavior with this flag on or off?)

Also is this related to #3883?

gaearon · 2018-09-30T21:41:09Z

How can I test this?

arcanis · 2018-09-30T22:54:59Z

I found a way to avoid symlinks: false: I intercept SymlinkPlugin directly from within the PnP plugin, and inhibits its effects when under PnP environments. Since the plugin is a noop otherwise, it won't affect anything else.

As for testing I didn't test directly on create-react-app, but I got a case where symlinks: false was causing issues with ts-loader, the plugin upgrade fixed the issue. You can check a minimal repro here: https://github.com/arcanis/webpack-symlinks-test/blob/master/webpack.config.js.

gaearon · 2018-09-30T22:58:57Z

Ah that sounds fine, thanks. I meant — how can I test this PR? Do I need to build yarn from master?

arcanis · 2018-10-01T09:14:34Z

The nightly Yarn should be fine: https://nightly.yarnpkg.com/yarn-1.12.0-20180928.1315.js

To enable PnP when creating a new project, you can set the YARN_PLUGNPLAY_OVERRIDE=1 environment variable for the create-react-app process (it won't add the installConfig.pnp key in the package.json, but will still tell Yarn to install everything using PnP regardless of the settings).

gaearon · 2018-10-01T13:33:03Z

Do we know why AppVeyor build is timing out?

Timer · 2018-10-01T13:41:20Z

Do we know why AppVeyor build is timing out?

Appears to have started with 2a7346e, though it did not add any new tests for AppVeyor -- strictly Travis.

Maybe the verdaccio upgrade caused something?

gaearon · 2018-10-01T14:53:13Z

Thanks!

gaearon · 2018-10-01T21:27:27Z

 collectCoverageFrom: ['src/**/*.{js,jsx}'],
- setupFiles: ['react-app-polyfill/jsdom'],
+ resolver: require.resolve('jest-pnp-resolver'),
+ setupFiles: [require.resolve('react-app-polyfill/jsdom')],


This leaves an absolute path after ejecting.

* Adds the PnP plugin for Webpack to find dependencies when working under PnP * Adds configuration for jest * Adds an e2e test for when using PnP * Avoids cra from crashing at the engine check * Avoids cra from crashing when initializing react-scripts * Makes the ownPath portable * Fixes linting * Bumps to pnp-webpack-plugin@1.1.0, removes symlinks: false * Adds a --use-pnp option * Pin version

facebook-github-bot added the CLA Signed label Sep 27, 2018

arcanis mentioned this pull request Sep 27, 2018
Yarn PnP Compatibility #5117
Closed

Timer added this to the 2.0.0 milestone Sep 27, 2018

arcanis force-pushed the pnp branch from 92bb0a5 to 539b600Compare September 27, 2018 16:49

Timer mentioned this pull request Sep 27, 2018
Remove runtime alias hack #5142
Merged

Timer modified the milestones: 2.0.0, 2.x Sep 28, 2018

arcanis mentioned this pull request Sep 28, 2018
Fixes workbox dependencies GoogleChrome/workbox#1667
Merged

arcanis force-pushed the pnp branch from 539b600 to ff4d177Compare September 28, 2018 15:06

arcanis mentioned this pull request Sep 28, 2018
Forwards args through thread-loader #5157
Merged

arcanis force-pushed the pnp branch from ff4d177 to 1d148e5Compare September 28, 2018 15:35

Maël Nison added 2 commits September 28, 2018 17:36

Adds the PnP plugin for Webpack to find dependencies when working und…
f04f0dd
…er PnP

Adds configuration for jest
63152ef

arcanis force-pushed the pnp branch from 1d148e5 to 63152efCompare September 28, 2018 15:39

Adds an e2e test for when using PnP
6eb3f33

arcanis force-pushed the pnp branch from f07537d to 6eb3f33Compare September 28, 2018 16:58

Maël Nison added 4 commits September 29, 2018 01:05

Avoids cra from crashing at the engine check
7c362b0

Avoids cra from crashing when initializing react-scripts
eae3d82

Makes the ownPath portable
5bbe63a

Fixes linting
747ba0e

arcanis force-pushed the pnp branch from 7927a5f to 747ba0eCompare September 29, 2018 12:23

gaearon reviewed Sep 29, 2018
View reviewed changes

Bumps to pnp-webpack-plugin@1.1.0, removes symlinks: false
3609e33

Adds a --use-pnp option
6862713

arcanis force-pushed the pnp branch from 64f59d1 to 6862713Compare October 1, 2018 14:21

Pin version
543f5f8

gaearon merged commit 9e074bb into react:masterOct 1, 2018

gaearon modified the milestones: 2.x, 2.0 Oct 1, 2018

Timer added the tag: enhancement label Oct 1, 2018

gaearon reviewed Oct 1, 2018
View reviewed changes

gaearon mentioned this pull request Oct 1, 2018
Fix absolute paths on eject #5214
Merged

msokk mentioned this pull request Nov 1, 2018
Absolute module imports through node_modules symlinks with Plug'n'Play yarnpkg/yarn#6616
Closed

edmorley mentioned this pull request Jan 4, 2019
Add support for Yarn's Plug n Play mode neutrinojs/neutrino#1277
Closed

lockBot locked and limited conversation to collaborators Jan 19, 2019


		const nodeArgs = fs.existsSync(pnpPath) ? ['--require', pnpPath] : [];

		await executeNodeScript(

Uh oh!

Uh oh!

Conversation

arcanis commented Sep 27, 2018

Uh oh!

Timer commented Sep 27, 2018

Uh oh!

arcanis commented Sep 27, 2018

Uh oh!

arcanis commented Sep 27, 2018

Uh oh!

Timer commented Sep 27, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arcanis commented Sep 27, 2018

Uh oh!

Timer commented Sep 27, 2018

Uh oh!

arcanis commented Sep 29, 2018

Uh oh!

gaearon commented Sep 29, 2018

Uh oh!

gaearon commented Sep 29, 2018

Uh oh!

gaearonSep 29, 2018

Choose a reason for hiding this comment

Uh oh!

arcanisSep 30, 2018

Choose a reason for hiding this comment

Uh oh!

gaearonSep 29, 2018

Choose a reason for hiding this comment

Uh oh!

TimerSep 29, 2018

Choose a reason for hiding this comment

Uh oh!

arcanisSep 30, 2018

Choose a reason for hiding this comment

Uh oh!

gaearonSep 30, 2018

Choose a reason for hiding this comment

Uh oh!

arcanis commented Sep 30, 2018

Uh oh!

gaearon commented Sep 30, 2018

Uh oh!

gaearon commented Sep 30, 2018

Uh oh!

arcanis commented Sep 30, 2018

Uh oh!

gaearon commented Sep 30, 2018

Uh oh!

arcanis commented Oct 1, 2018

Uh oh!

gaearon commented Oct 1, 2018

Uh oh!

Timer commented Oct 1, 2018

Uh oh!

gaearon commented Oct 1, 2018

Uh oh!

gaearonOct 1, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Timer commented Sep 27, 2018•
edited
Loading