Skip to content

feat: react-scripts lint command #1217 #3850

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

Closed
wants to merge 1 commit into from
Closed

feat: react-scripts lint command #1217 #3850

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
10 changes: 8 additions & 2 deletions packages/react-scripts/bin/react-scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@ const spawn = require('react-dev-utils/crossSpawn');
const args = process.argv.slice(2);

const scriptIndex = args.findIndex(
x => x === 'build' || x === 'eject' || x === 'start' || x === 'test'
x =>
x === 'build' ||
x === 'eject' ||
x === 'start' ||
x === 'test' ||
x === 'lint'
);
const script = scriptIndex === -1 ? args[0] : args[scriptIndex];
const nodeArgs = scriptIndex > 0 ? args.slice(0, scriptIndex) : [];
Expand All @@ -28,7 +33,8 @@ switch (script) {
case 'build':
case 'eject':
case 'start':
case 'test': {
case 'test':
case 'lint': {
const result = spawn.sync(
'node',
nodeArgs
Expand Down
28 changes: 28 additions & 0 deletions packages/react-scripts/scripts/lint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// @remove-on-eject-begin
/**
* Copyright (c) 2015-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
// @remove-on-eject-end
'use strict';

const CLIEngine = require('eslint').CLIEngine;

const cli = new CLIEngine({
// @remove-on-eject-begin
configFile: require.resolve('eslint-config-react-app'),
Copy link
Contributor

Choose a reason for hiding this comment

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

hardcoding this won't respect the changes that user made in package.json or .eslintrc post eject. IMO we should wrap this line:

// @remove-on-eject-begin
    configFile: require.resolve('eslint-config-react-app'),
// @remove-on-eject-end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes ... but then react-scripts lint after eject has few errors:

/private/tmp/demo20/src/App.js
  1:1   warning  Definition for rule 'jsx-a11y/href-no-hash' was not found  jsx-a11y/href-no-hash
 
/private/tmp/demo20/src/App.test.js
  1:1  warning  Definition for rule 'jsx-a11y/href-no-hash' was not found  jsx-a11y/href-no-hash

/private/tmp/demo20/src/index.js
  1:1  warning  Definition for rule 'jsx-a11y/href-no-hash' was not found  jsx-a11y/href-no-hash

/private/tmp/demo20/src/registerServiceWorker.js
  1:1  warning  Definition for rule 'jsx-a11y/href-no-hash' was not found  jsx-a11y/href-no-hash

✖ 4 problems (0 errors, 4 warnings)

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, do you have eslint installed globally / .eslintrc file somewhere in the parent folder? i think we need to add root: true to eslintConfig property in package.json post eject. https://eslint.org/docs/user-guide/configuring#using-configuration-files can you try adding it and see if it fix the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly, no change after:

  "eslintConfig": {
    "root": true,
    "extends": "react-app"
  }

Copy link
Contributor Author

@maciej-ka maciej-ka Jan 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also adding plugins: ['jsx-a11y'] to CLIEngine constructor options doesn't remove errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe running it with DEBUG=eslint:* yarn lint would help you found the problem. I cannot reproduce it locally using the same code :(

Copy link
Contributor

Choose a reason for hiding this comment

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

or DEBUG=eslint:config* yarn lint if you just want the log related to config

// @remove-on-eject-end
fix: process.argv.slice(2).indexOf('--fix') >= 0,
});
const report = cli.executeOnFiles(['src/**/*.{js,jsx,mjs}']);
const formatter = cli.getFormatter();

// persist changed files when using --fix option
CLIEngine.outputFixes(report);
console.log(formatter(report.results));

if (report.errorCount > 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Condition to trigger exit(1) could be different when a --fix flag was used.

Report object has a count of fixable errors and it is possible to count how many unfixed errors are left:

{
  '...': '...',
  errorCount: 1,
  fixableErrorCount: 1,
}

My concern is that in react-scripts lint --fix && git push files will be changed by lint but they still need git commit.

process.exit(1);
}
6 changes: 6 additions & 0 deletions tasks/e2e-simple.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,9 @@ exists build/static/css/*.css
exists build/static/media/*.svg
exists build/favicon.ico

# Test linter
./node_modules/.bin/react-scripts lint

# Run tests with CI flag
CI=true yarn test
# Uncomment when snapshot testing is enabled by default:
Expand Down Expand Up @@ -273,6 +276,9 @@ exists build/static/css/*.css
exists build/static/media/*.svg
exists build/favicon.ico

# Test linter
node scripts/lint.js

# Run tests, overring the watch option to disable it.
# `CI=true yarn test` won't work here because `yarn test` becomes just `jest`.
# We should either teach Jest to respect CI env variable, or make
Expand Down