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

Plug'n'Play support #5136

Merged
merged 10 commits into from
Oct 1, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ cache:
directories:
- .npm
before_install:
- curl -o- -L https://yarnpkg.com/install.sh | bash
- curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --nightly
- export PATH="$HOME/.yarn/bin:$PATH"
install: true
script:
Expand Down
2 changes: 1 addition & 1 deletion appveyor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ platform:
install:
- ps: Install-Product node $env:nodejs_version $env:platform
- ps: |
(New-Object Net.WebClient).DownloadFile("https://yarnpkg.com/latest.msi", "$env:temp\yarn.msi")
(New-Object Net.WebClient).DownloadFile("https://nightly.yarnpkg.com/latest.msi", "$env:temp\yarn.msi")
cmd /c start /wait msiexec.exe /i $env:temp\yarn.msi /quiet /qn /norestart

build: off
Expand Down
49 changes: 40 additions & 9 deletions packages/create-react-app/createReactApp.js
Original file line number Diff line number Diff line change
Expand Up @@ -340,19 +340,25 @@ function run(
() => packageName
);
})
.then(packageName => {
.then(async packageName => {
checkNodeVersion(packageName);
setCaretRangeForRuntimeDeps(packageName);

const scriptsPath = path.resolve(
process.cwd(),
'node_modules',
packageName,
'scripts',
'init.js'
const pnpPath = path.resolve(process.cwd(), '.pnp.js');

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

await executeNodeScript(
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work on Node LTS version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

{
cwd: process.cwd(),
args: nodeArgs,
},
[root, appName, verbose, originalDirectory, template],
`
var init = require('${packageName}/scripts/init.js');
init.apply(null, JSON.parse(process.argv[1]));
`
);
const init = require(scriptsPath);
init(root, appName, verbose, originalDirectory, template);

if (version === 'react-scripts@0.9.x') {
console.log(
Expand Down Expand Up @@ -540,6 +546,11 @@ function checkNodeVersion(packageName) {
packageName,
'package.json'
);

if (!fs.existsSync(packageJsonPath)) {
return;
}

const packageJson = require(packageJsonPath);
if (!packageJson.engines || !packageJson.engines.node) {
return;
Expand Down Expand Up @@ -794,3 +805,23 @@ function checkIfOnline(useYarn) {
});
});
}

function executeNodeScript({ cwd, args }, data, source) {
return new Promise((resolve, reject) => {
const child = spawn(
process.execPath,
[...args, '-e', source, '--', JSON.stringify(data)],
{ cwd, stdio: 'inherit' }
);

child.on('close', code => {
if (code !== 0) {
reject({
command: `node ${args.join(' ')}`,
});
return;
}
resolve();
});
});
}
17 changes: 17 additions & 0 deletions packages/react-scripts/config/webpack.config.dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

const path = require('path');
const webpack = require('webpack');
const PnpWebpackPlugin = require('pnp-webpack-plugin');
const HtmlWebpackPlugin = require('html-webpack-plugin');
const CaseSensitivePathsPlugin = require('case-sensitive-paths-webpack-plugin');
const InterpolateHtmlPlugin = require('react-dev-utils/InterpolateHtmlPlugin');
Expand Down Expand Up @@ -149,13 +150,29 @@ module.exports = {
'react-native': 'react-native-web',
},
plugins: [
// Adds support for installing with Plug'n'Play, leading to faster installs and adding
// guards against forgotten dependencies and such.
PnpWebpackPlugin,
// 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

},
resolveLoader: {
plugins: [
// Also related to Plug'n'Play, but this time it tells Webpack to load its loaders
// from the current package.
PnpWebpackPlugin.moduleLoader(module),
],
// 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,
},
module: {
strictExportPresence: true,
Expand Down
17 changes: 17 additions & 0 deletions packages/react-scripts/config/webpack.config.prod.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

const path = require('path');
const webpack = require('webpack');
const PnpWebpackPlugin = require('pnp-webpack-plugin');
const HtmlWebpackPlugin = require('html-webpack-plugin');
const InlineChunkHtmlPlugin = require('react-dev-utils/InlineChunkHtmlPlugin');
const TerserPlugin = require('terser-webpack-plugin');
Expand Down Expand Up @@ -204,13 +205,29 @@ module.exports = {
'react-native': 'react-native-web',
},
plugins: [
// Adds support for installing with Plug'n'Play, leading to faster installs and adding
// guards against forgotten dependencies and such.
PnpWebpackPlugin,
// 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,
},
resolveLoader: {
plugins: [
// Also related to Plug'n'Play, but this time it tells Webpack to load its loaders
// from the current package.
PnpWebpackPlugin.moduleLoader(module),
],
// 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,
},
module: {
strictExportPresence: true,
Expand Down
3 changes: 3 additions & 0 deletions packages/react-scripts/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,11 @@
"html-webpack-plugin": "4.0.0-alpha.2",
"identity-obj-proxy": "3.0.0",
"jest": "23.6.0",
"jest-pnp-resolver": "^1.0.1",
"jest-resolve": "23.6.0",
"mini-css-extract-plugin": "0.4.3",
"optimize-css-assets-webpack-plugin": "5.0.1",
"pnp-webpack-plugin": "1.0.2",
"postcss-flexbugs-fixes": "4.1.0",
"postcss-loader": "3.0.0",
"postcss-preset-env": "6.0.6",
Expand Down
6 changes: 3 additions & 3 deletions packages/react-scripts/scripts/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ module.exports = function(
originalDirectory,
template
) {
const ownPackageName = require(path.join(__dirname, '..', 'package.json'))
.name;
const ownPath = path.join(appPath, 'node_modules', ownPackageName);
const ownPath = path.dirname(
require.resolve(path.join(__dirname, '..', 'package.json'))
);
const appPackage = require(path.join(appPath, 'package.json'));
const useYarn = fs.existsSync(path.join(appPath, 'yarn.lock'));

Expand Down
3 changes: 2 additions & 1 deletion packages/react-scripts/scripts/utils/createJestConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')],
Copy link
Contributor

Choose a reason for hiding this comment

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

This leaves an absolute path after ejecting.

setupTestFrameworkScriptFile: setupTestsFile,
testMatch: [
'<rootDir>/src/**/__tests__/**/*.{js,jsx}',
Expand Down
12 changes: 12 additions & 0 deletions tasks/e2e-installs.sh
Original file line number Diff line number Diff line change
Expand Up @@ -229,5 +229,17 @@ npx create-react-app test-app-nested-paths-t3/aa/bb/cc/dd
cd test-app-nested-paths-t3/aa/bb/cc/dd
yarn start --smoke-test

# ******************************************************************************
# Test when PnP is enabled
# ******************************************************************************
cd "$temp_app_path"
echo $OSTYPE
YARN_PLUGNPLAY_OVERRIDE=1 npx create-react-app test-app-pnp
cd test-app-pnp
! exists node_modules
exists .pnp.js
yarn start --smoke-test
yarn build

# Cleanup
cleanup