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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plug'n'Play support #5136

Merged
merged 10 commits into from Oct 1, 2018
Merged

Plug'n'Play support #5136

merged 10 commits into from Oct 1, 2018

Conversation

@arcanis
Copy link
Contributor

arcanis commented Sep 27, 2018

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 馃檪

screen shot 2018-09-27 at 4 52 09 pm

@Timer

This comment has been minimized.

Copy link
Collaborator

Timer commented Sep 27, 2018

We probably need the Jest resolver too, right?

@arcanis

This comment has been minimized.

Copy link
Contributor Author

arcanis commented Sep 27, 2018

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 馃憤

@Timer Timer added this to the 2.0.0 milestone Sep 27, 2018
@arcanis

This comment has been minimized.

Copy link
Contributor Author

arcanis commented Sep 27, 2018

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

This comment has been minimized.

Copy link
Collaborator

Timer commented Sep 27, 2018

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

@arcanis arcanis force-pushed the arcanis:pnp branch from 92bb0a5 to 539b600 Sep 27, 2018
@arcanis

This comment has been minimized.

Copy link
Contributor Author

arcanis commented Sep 27, 2018

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

This comment has been minimized.

Copy link
Collaborator

Timer commented Sep 27, 2018

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

@Timer Timer mentioned this pull request Sep 27, 2018
@Timer Timer modified the milestones: 2.0.0, 2.x Sep 28, 2018
@arcanis arcanis force-pushed the arcanis:pnp branch from 539b600 to ff4d177 Sep 28, 2018
@arcanis arcanis force-pushed the arcanis:pnp branch from ff4d177 to 1d148e5 Sep 28, 2018
@arcanis arcanis force-pushed the arcanis:pnp branch from 1d148e5 to 63152ef Sep 28, 2018
Ma毛l Nison
@arcanis arcanis force-pushed the arcanis:pnp branch from f07537d to 6eb3f33 Sep 28, 2018
Ma毛l Nison added 4 commits Sep 28, 2018
@arcanis arcanis force-pushed the arcanis:pnp branch from 7927a5f to 747ba0e Sep 29, 2018
@arcanis

This comment has been minimized.

Copy link
Contributor Author

arcanis commented Sep 29, 2018

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

This comment has been minimized.

Copy link
Member

gaearon commented Sep 29, 2018

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

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Sep 29, 2018

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?


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

await executeNodeScript(

This comment has been minimized.

Copy link
@gaearon

gaearon Sep 29, 2018

Member

Does this work on Node LTS version?

This comment has been minimized.

Copy link
@arcanis

arcanis Sep 30, 2018

Author Contributor

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

// Prevents users from importing files from outside of src/ (or node_modules/).
// This often causes confusion because we only process files within src/ with babel.
// To fix this, we prevent you from importing files out of src/ -- if you'd like to,
// please link the files into your node_modules/ and let module-resolution kick in.
// Make sure your source files are compiled, as they will not be processed in any way.
new ModuleScopePlugin(paths.appSrc, [paths.appPackageJson]),
],
// 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,

This comment has been minimized.

Copy link
@gaearon

gaearon Sep 29, 2018

Member

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?

This comment has been minimized.

Copy link
@Timer

Timer Sep 29, 2018

Collaborator

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.

This comment has been minimized.

Copy link
@arcanis

arcanis Sep 30, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@gaearon

gaearon Sep 30, 2018

Member

We didn鈥檛 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

This comment has been minimized.

Copy link
Contributor Author

arcanis commented Sep 30, 2018

@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

This comment has been minimized.

Copy link
Member

gaearon commented Sep 30, 2018

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鈥檓 not sure they鈥檙e valid in the first place. I鈥檓 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

This comment has been minimized.

Copy link
Member

gaearon commented Sep 30, 2018

How can I test this?

@arcanis

This comment has been minimized.

Copy link
Contributor Author

arcanis commented Sep 30, 2018

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

This comment has been minimized.

Copy link
Member

gaearon commented Sep 30, 2018

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

@arcanis

This comment has been minimized.

Copy link
Contributor Author

arcanis commented Oct 1, 2018

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

This comment has been minimized.

Copy link
Member

gaearon commented Oct 1, 2018

Do we know why AppVeyor build is timing out?

@Timer

This comment has been minimized.

Copy link
Collaborator

Timer commented Oct 1, 2018

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?

Ma毛l Nison
@arcanis arcanis force-pushed the arcanis:pnp branch from 64f59d1 to 6862713 Oct 1, 2018
@gaearon gaearon merged commit 9e074bb into facebook:master Oct 1, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Oct 1, 2018

Thanks!

@gaearon gaearon modified the milestones: 2.x, 2.0 Oct 1, 2018
@@ -22,7 +22,8 @@ module.exports = (resolve, rootDir, isEjecting) => {
// in Jest configs. We need help from somebody with Windows to determine this.
const config = {
collectCoverageFrom: ['src/**/*.{js,jsx}'],
setupFiles: ['react-app-polyfill/jsdom'],
resolver: require.resolve('jest-pnp-resolver'),
setupFiles: [require.resolve('react-app-polyfill/jsdom')],

This comment has been minimized.

Copy link
@gaearon

gaearon Oct 1, 2018

Member

This leaves an absolute path after ejecting.

zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Oct 2, 2018
* 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
mrmckeb pushed a commit to mrmckeb/create-react-app that referenced this pull request Nov 6, 2018
* 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
@lock lock bot locked and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.