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

Jump to bottom

Add opt-out for eslint-webpack-plugin #10170

Merged
merged 1 commit into from Feb 3, 2021
Merged

Add opt-out for eslint-webpack-plugin #10170

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions docusaurus/docs/advanced-configuration.md
View file
Open in desktop
Expand Up @@ -26,4 +26,6 @@ You can adjust various development and production settings by setting environmen
| IMAGE_INLINE_SIZE_LIMIT | 🚫 Ignored | ✅ Used | By default, images smaller than 10,000 bytes are encoded as a data URI in base64 and inlined in the CSS or JS build artifact. Set this to control the size limit in bytes. Setting it to 0 will disable the inlining of images. |
| FAST_REFRESH | ✅ Used | 🚫 Ignored | When set to `false`, disables experimental support for Fast Refresh to allow you to tweak your components in real time without reloading the page. |
| TSC_COMPILE_ON_ERROR | ✅ Used | ✅ Used | When set to `true`, you can run and properly build TypeScript projects even if there are TypeScript type check errors. These errors are printed as warnings in the terminal and/or browser console. |
| ESLINT_NO_DEV_ERRORS | ✅ Used | 🚫 Ignored | When set to `true`, ESLint errors are converted to warnings during development. As a result, ESLint output will no longer appear in the error overlay. |
| DISABLE_ESLINT_PLUGIN | ✅ Used | ✅ Used | When set to `true`, [eslint-webpack-plugin](https://github.com/webpack-contrib/eslint-webpack-plugin) will be completely disabled. |
| DISABLE_NEW_JSX_TRANSFORM | ✅ Used | ✅ Used | When set to `true`, disables the [new JSX transform](https://reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html) introduced in React 17 and backported to React 16.14.0, 15.7.0, and 0.14.10. New projects will use a version of React that supports this by default but you may need to disable it in existing projects if you can't upgrade React. |
6 changes: 5 additions & 1 deletion packages/react-dev-utils/eslintFormatter.js
View file
Open in desktop
Expand Up @@ -14,6 +14,10 @@ const table = require('text-table');

const cwd = process.cwd();

const emitErrorsAsWarnings =
process.env.NODE_ENV === 'development' &&
process.env.ESLINT_NO_DEV_ERRORS === 'true';

function isError(message) {
if (message.fatal || message.severity === 2) {
return true;
Expand All @@ -38,7 +42,7 @@ function formatter(results) {

messages = messages.map(message => {
let messageType;
if (isError(message)) {
if (isError(message) && !emitErrorsAsWarnings) {
messageType = 'error';
hasErrors = true;
if (message.ruleId) {
Expand Down
49 changes: 27 additions & 22 deletions packages/react-scripts/config/webpack.config.js
View file
Open in desktop
Expand Up @@ -55,6 +55,9 @@ const reactRefreshOverlayEntry = require.resolve(
// makes for a smoother build process.
const shouldInlineRuntimeChunk = process.env.INLINE_RUNTIME_CHUNK !== 'false';

const emitErrorsAsWarnings = process.env.ESLINT_NO_DEV_ERRORS === 'true';
const disableESLintPlugin = process.env.DISABLE_ESLINT_PLUGIN === 'true';

const imageInlineSizeLimit = parseInt(
process.env.IMAGE_INLINE_SIZE_LIMIT || '10000'
);
Expand Down Expand Up @@ -750,29 +753,31 @@ module.exports = function (webpackEnv) {
// The formatter is invoked directly in WebpackDevServerUtils during development
formatter: isEnvProduction ? typescriptFormatter : undefined,
}),
new ESLintPlugin({
// Plugin options
extensions: ['js', 'mjs', 'jsx', 'ts', 'tsx'],
formatter: require.resolve('react-dev-utils/eslintFormatter'),
eslintPath: require.resolve('eslint'),
context: paths.appSrc,
cache: true,
cacheLocation: path.resolve(
paths.appNodeModules,
'.cache/.eslintcache'
),
// ESLint class options
cwd: paths.appPath,
resolvePluginsRelativeTo: __dirname,
baseConfig: {
extends: [require.resolve('eslint-config-react-app/base')],
rules: {
...(!hasJsxRuntime && {
'react/react-in-jsx-scope': 'error',
}),
!disableESLintPlugin &&
new ESLintPlugin({
// Plugin options
extensions: ['js', 'mjs', 'jsx', 'ts', 'tsx'],
formatter: require.resolve('react-dev-utils/eslintFormatter'),
eslintPath: require.resolve('eslint'),
emitWarning: isEnvDevelopment && emitErrorsAsWarnings,
Copy link

Choose a reason for hiding this comment

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

I think adding this emitWarning flag had the unintended effect of hiding warnings entirely if ESLINT_NO_DEV_ERRORS is not true. I was able to confirm that the warnings correctly appear if I set that env variable but then eslint errors are downgraded to warnings. I dug around the eslint-webpack-plugin codebase a bit and I think we might not even need to set this option since the default is true already.

There was also a bug fix recently in eslint-webpack-plugin@2.5.0 to fix an issue with emitWarning and emitError so react-scripts might also want to bump up that version requirement if this option is staying in.

Copy link

Choose a reason for hiding this comment

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

I read through the linked issue and I think the failOnError option was the one people wanted here - maybe something like this?

Suggested change
emitWarning: isEnvDevelopment && emitErrorsAsWarnings,
...(isEnvDevelopment && emitErrorsAsWarnings && { failOnError: false }),

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 @DarkAce65, are you able to provide a reproduction of this?

Our error logging won't show warnings when errors are present (as far as I remember), so could that be what you mean?

I can see locally that if I have only a warning, no errors, I see that warning. If I have errors and warnings, I see only errors.

Copy link

Choose a reason for hiding this comment

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

Hey @mrmckeb, that's odd, it's not what I was seeing. You're right that if there's an error, no warnings show up but I'm also not seeing any warnings if there are no errors :(

There's an example in #10509 but I can try and make a reproduction repo in a few hours.

Out of curiosity, what version of eslint-webpack-plugin do you have locally? Also, you have ESLINT_NO_DEV_ERRORS set to false right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the update, I have eslint-webpack-plugin@2.4.3.

If I run npx create-react-app --info, I see:

  Binaries:
    Node: 14.15.0 - ~/.nvm/versions/node/v14.15.0/bin/node
    Yarn: 1.22.5 - /usr/local/bin/yarn
    npm: 6.14.8 - ~/.nvm/versions/node/v14.15.0/bin/npm
  Browsers:
    Chrome: 88.0.4324.150
    Edge: Not Found
    Firefox: Not Found
    Safari: 14.0.3
  npmPackages:
    react: ^17.0.1 => 17.0.1 
    react-dom: ^17.0.1 => 17.0.1 
    react-scripts: 4.0.2 => 4.0.2 
  npmGlobalPackages:
    create-react-app: Not Found

Copy link

@DarkAce65 DarkAce65 Feb 12, 2021 •
edited

Choose a reason for hiding this comment

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

Did a bit more testing, seems like the combination of ESLINT_NO_DEV_ERRORS being false or unset, react-scripts@4.0.2, and eslint-webpack-plugin@2.5.0 causes the missing warnings: https://github.com/DarkAce65/cra-eslint-warnings

context: paths.appSrc,
cache: true,
cacheLocation: path.resolve(
paths.appNodeModules,
'.cache/.eslintcache'
),
// ESLint class options
cwd: paths.appPath,
resolvePluginsRelativeTo: __dirname,
baseConfig: {
extends: [require.resolve('eslint-config-react-app/base')],
rules: {
...(!hasJsxRuntime && {
'react/react-in-jsx-scope': 'error',
}),
},
},
},
}),
}),
].filter(Boolean),
// Some libraries import Node modules but don't use them in the browser.
// Tell webpack to provide empty mocks for them so importing them works.
Expand Down