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.

Sign up for GitHub

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

fix: resolve new JSX transform issues #9788

Merged
merged 1 commit into from
Oct 15, 2020
Merged

fix: resolve new JSX transform issues #9788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 10 additions & 5 deletions packages/babel-preset-react-app/create.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@

const path = require('path');

const hasJsxRuntime = (() => {
try {
require.resolve('react/jsx-runtime.js');
return true;
} catch (e) {
return false;
}
})();

const validateBoolOption = (name, value, defaultValue) => {
if (typeof value === 'undefined') {
value = defaultValue;
Expand Down Expand Up @@ -54,10 +63,6 @@ module.exports = function (api, opts, env) {
);
}

var hasJsxRuntime = Boolean(
api.caller(caller => !!caller && caller.hasJsxRuntime)
);

if (!isEnvDevelopment && !isEnvProduction && !isEnvTest) {
throw new Error(
'Using `babel-preset-react-app` requires that you specify `NODE_ENV` or ' +
Expand Down Expand Up @@ -100,7 +105,7 @@ module.exports = function (api, opts, env) {
// Will use the native built-in instead of trying to polyfill
// behavior for any plugins that require one.
...(!hasJsxRuntime ? { useBuiltIns: true } : {}),
runtime: opts.runtime || 'classic',
runtime: hasJsxRuntime ? 'automatic' : 'classic',
Copy link
Contributor

Choose a reason for hiding this comment

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

is anything going to break by removing the option to pass in runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that was introduced for v4 and hasn't been released yet. @iansu can confirm though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. This doesn't look right. I think it is dangerous to check right in the preset. React might not necessarily be resolvable from that path (e.g. a monorepo setup). I think it was correct to keep this an option in the preset.

Copy link
Contributor

Choose a reason for hiding this comment

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

It also makes the preset's behavior non-deterministic depending on whether you install React before or after you first compile you code. Wrong output might get cached.

Copy link
Contributor

Choose a reason for hiding this comment

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

#9822

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @gaearon, we discussed the risk around resolution on the last call, but weren't sure of the right path forward. I've asked @ianschmitz and @iansu for their thoughts on the new issue you've raised too - thanks!

},
],
isTypeScriptEnabled && [require('@babel/preset-typescript').default],
Expand Down
15 changes: 13 additions & 2 deletions packages/eslint-config-react-app/base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@
// React App support, and is used as the `baseConfig` for `eslint-loader`
// to ensure that user-provided configs don't need this boilerplate.

const hasJsxRuntime = (() => {
try {
require.resolve('react/jsx-runtime.js');
return true;
} catch (e) {
return false;
}
})();

module.exports = {
root: true,

Expand Down Expand Up @@ -41,8 +50,10 @@ module.exports = {
},

rules: {
'react/jsx-uses-react': 'warn',
'react/jsx-uses-vars': 'warn',
'react/react-in-jsx-scope': 'error',
...(!hasJsxRuntime && {
'react/jsx-uses-react': 'warn',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the blog post, it won't hurt if users still import React, however they don't need to do it if the version of React they have supports the new transform.

So I think it's correct to flag React as an unused import in this scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also worth noting that this may mislead users who have the new transform installed, but aren't using Create React App and haven't configured Babel for the new transform.

If we release the ESLint config as a major, and add that to the release notes, I think we'll be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that opening a project to see a hundred lint warnings would be a bad experience. Especially if people don’t realize there is a codemod they can run. They will also be copy-pasting code from the web that has this import. How do we plan to explain this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I almost think it shouldn’t warn in the transitional period. We can tighten it up later as a separate breaking change.

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 had it that way - then flipped it, as I wasn't sure. We can swap that back of course.

This will be a major version, so we do have the opportunity to detail these changes (and why) in the release notes - but I understand not everyone reads those. We could also look to add a custom ESLint rule for this - but that seems like overkill.

Copy link
Contributor Author

@mrmckeb mrmckeb Oct 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This has been changed as a part of #9751, @gaearon.

We can change this later, and I'll reach out to the eslint-plugin-react team about this too, as it might make sense to have the rule be 'smarter' in future.

'react/react-in-jsx-scope': 'error',
}),
},
};
13 changes: 1 addition & 12 deletions packages/react-scripts/config/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const fs = require('fs');
const path = require('path');
const webpack = require('webpack');
const resolve = require('resolve');
const semver = require('semver');
const PnpWebpackPlugin = require('pnp-webpack-plugin');
const HtmlWebpackPlugin = require('html-webpack-plugin');
const CaseSensitivePathsPlugin = require('case-sensitive-paths-webpack-plugin');
Expand All @@ -34,7 +33,6 @@ const ModuleNotFoundPlugin = require('react-dev-utils/ModuleNotFoundPlugin');
const ForkTsCheckerWebpackPlugin = require('react-dev-utils/ForkTsCheckerWebpackPlugin');
const typescriptFormatter = require('react-dev-utils/typescriptFormatter');
const ReactRefreshWebpackPlugin = require('@pmmmwh/react-refresh-webpack-plugin');
const react = require(require.resolve('react', { paths: [paths.appPath] }));
// @remove-on-eject-begin
const getCacheIdentifier = require('react-dev-utils/getCacheIdentifier');
// @remove-on-eject-end
Expand Down Expand Up @@ -403,16 +401,7 @@ module.exports = function (webpackEnv) {
// @remove-on-eject-begin
babelrc: false,
configFile: false,
presets: [
[
require.resolve('babel-preset-react-app'),
{
runtime: semver.gte(react.version, '17.0.0-alpha.0')
? 'automatic'
: 'classic',
},
],
],
presets: [require.resolve('babel-preset-react-app')],
// Make sure we have a unique cache identifier, erring on the
// side of caution.
// We remove this when the user ejects because the default
Expand Down