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 all 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
89 changes: 73 additions & 16 deletions packages/create-react-app/createReactApp.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ const program = new commander.Command(packageJson.name)
'use a non-standard version of react-scripts'
)
.option('--use-npm')
.option('--use-pnp')
.allowUnknownOption()
.on('--help', () => {
console.log(` Only ${chalk.green('<project-directory>')} is required.`);
Expand Down Expand Up @@ -178,10 +179,11 @@ createApp(
program.verbose,
program.scriptsVersion,
program.useNpm,
program.usePnp,
hiddenProgram.internalTestingTemplate
);

function createApp(name, verbose, version, useNpm, template) {
function createApp(name, verbose, version, useNpm, usePnp, template) {
const root = path.resolve(name);
const appName = path.basename(root);

Expand Down Expand Up @@ -241,7 +243,16 @@ function createApp(name, verbose, version, useNpm, template) {
version = 'react-scripts@0.9.x';
}
}
run(root, appName, version, verbose, originalDirectory, template, useYarn);
run(
root,
appName,
version,
verbose,
originalDirectory,
template,
useYarn,
usePnp
);
}

function shouldUseYarn() {
Expand All @@ -253,7 +264,7 @@ function shouldUseYarn() {
}
}

function install(root, useYarn, dependencies, verbose, isOnline) {
function install(root, useYarn, usePnp, dependencies, verbose, isOnline) {
return new Promise((resolve, reject) => {
let command;
let args;
Expand All @@ -263,6 +274,9 @@ function install(root, useYarn, dependencies, verbose, isOnline) {
if (!isOnline) {
args.push('--offline');
}
if (usePnp) {
args.push('--enable-pnp');
}
[].push.apply(args, dependencies);

// Explicitly set cwd() to work around issues like
Expand All @@ -287,6 +301,12 @@ function install(root, useYarn, dependencies, verbose, isOnline) {
'--loglevel',
'error',
].concat(dependencies);

if (usePnp) {
console.log(chalk.yellow("NPM doesn't support PnP."));
console.log(chalk.yellow('Falling back to the regular installs.'));
console.log();
}
}

if (verbose) {
Expand All @@ -313,7 +333,8 @@ function run(
verbose,
originalDirectory,
template,
useYarn
useYarn,
usePnp
) {
const packageToInstall = getInstallPackage(version, originalDirectory);
const allDependencies = ['react', 'react-dom', packageToInstall];
Expand All @@ -336,23 +357,34 @@ function run(
);
console.log();

return install(root, useYarn, allDependencies, verbose, isOnline).then(
() => packageName
);
return install(
root,
useYarn,
usePnp,
allDependencies,
verbose,
isOnline
).then(() => 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 +572,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 +831,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();
});
});
}
11 changes: 11 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,6 +150,9 @@ 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,
Expand All @@ -157,6 +161,13 @@ module.exports = {
new ModuleScopePlugin(paths.appSrc, [paths.appPackageJson]),
],
},
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),
],
},
module: {
strictExportPresence: true,
rules: [
Expand Down
11 changes: 11 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,6 +205,9 @@ 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,
Expand All @@ -212,6 +216,13 @@ module.exports = {
new ModuleScopePlugin(paths.appSrc, [paths.appPackageJson]),
],
},
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),
],
},
module: {
strictExportPresence: true,
rules: [
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.1.0",
"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
11 changes: 11 additions & 0 deletions tasks/e2e-installs.sh
Original file line number Diff line number Diff line change
Expand Up @@ -229,5 +229,16 @@ 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"
npx create-react-app test-app-pnp --use-pnp
cd test-app-pnp
! exists node_modules
exists .pnp.js
yarn start --smoke-test
yarn build

# Cleanup
cleanup