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

Update top-level lint deps #1544

Closed
wants to merge 6 commits into from
Closed

Update top-level lint deps #1544

wants to merge 6 commits into from

Conversation

Timer
Copy link
Contributor

@Timer Timer commented Feb 12, 2017

No description provided.

package.json Outdated
"eslint": "^3.15.0",
"eslint-config-react-app": "0.5.1",
"eslint-plugin-flowtype": "^2.30.0",
"eslint-plugin-import": "^2.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just remove this one? I don't think we ever enabled it in the end, and there are still some issues preventing it from working well with eslint-loader (specifically, fixing the exporting file doesn't "recheck" the importing file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use the plugin in our eslint config so I don't think we can drop it, it's also a peer dep.

p.s. these packages are the top-level ones and strictly used for CI linting, they don't persist to the end-user in any way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I forgot about ''import/no-webpack-loader-syntax''. Okay

"eslint-plugin-import": "1.12.0",
"eslint-plugin-jsx-a11y": "2.2.2",
"eslint-plugin-react": "6.3.0",
"babel-eslint": "^7.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we enable ESLint's native support for async/await?
See note in https://github.com/babel/babel-eslint/releases/tag/v7.0.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.

This is a peerDep of our config, and we can't switch to ESLint's native support until import() lands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See import-js/eslint-plugin-import#700.

The babel-eslint parser is required for it to be parsed properly (syntax parsing is not the job of an eslint plugin).

package.json Outdated
"eslint-config-react-app": "0.5.1",
"eslint-plugin-flowtype": "^2.30.0",
"eslint-plugin-import": "^2.2.0",
"eslint-plugin-jsx-a11y": "2.2.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this one pinned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will unpin, what about our preset (eslint-config-react-app)? Lerna doesn't bump this automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why do we even have it in root package.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our CI currently runs lint in e2e-simple, see https://travis-ci.org/facebookincubator/create-react-app/jobs/201147269#L204-L234. We don't treat warnings as failures though, would you like to?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh okay. Yeah I think we should fail on them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can unpin there.

@gaearon
Copy link
Contributor

gaearon commented Feb 13, 2017

To answer your earlier question—we unpinned ESLint config in #1110.

Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Looks fine (but please unpin the ESLint plugin).
See other notes for optional changes.

@@ -15,7 +15,6 @@
// Learn more about creating plugins like this:
// https://github.com/ampedandwired/html-webpack-plugin#events

'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

@Timer Timer Feb 13, 2017

Choose a reason for hiding this comment

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

/home/travis/build/facebookincubator/create-react-app/packages/react-dev-utils/InterpolateHtmlPlugin.js
  18:1  warning  'use strict' is unnecessary inside of modules  strict

I can ignore it (like I did to the CLI), I was just listening to eslint. 😛 Was waiting to see if CI passed on node 4.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused why it thinks it's a module. We're not using import syntax anywhere.

Copy link
Contributor Author

@Timer Timer Feb 13, 2017

Choose a reason for hiding this comment

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

We tell it so, I'll put the stricts back.

  parserOptions: {
    ecmaVersion: 6,
>   sourceType: 'module',
    ecmaFeatures: {
      jsx: true,
      generators: true,
      experimentalObjectRestSpread: true
    }
  },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could override extending our own config, but I'm not sure if that's necessary.

@gaearon
Copy link
Contributor

gaearon commented Feb 13, 2017

We actually need to add stricts to Node-targeted code. Like react-scripts/config/*.

@Timer
Copy link
Contributor Author

Timer commented Feb 13, 2017

Most of these configs contain module.exports which eslint argues doesn't need 'use strict';, but I've adjusted the config for the other files -- what do you think now?

@gaearon
Copy link
Contributor

gaearon commented Feb 13, 2017

Most of these configs contain module.exports which eslint argues doesn't need 'use strict';

This is weird (and I think it's wrong). use strict is only unnecessary for code using ES Modules. It is necessary in Node. Can you point me to ESLint doc that says otherwise?

@Timer
Copy link
Contributor Author

Timer commented Feb 13, 2017

Sorry, I didn't mean to state that as fact -- I derived it's been a problem before from the conversation on eslint/eslint#4832, but nothing I change seems to be making it catch and require the 'use strict';. I'm not sure if there was a regression or some really weird configuration bug, but I'm stumped.

@gaearon
Copy link
Contributor

gaearon commented Mar 5, 2017

Instead let's do #1729.

@gaearon gaearon closed this Mar 5, 2017
@Timer Timer deleted the linter branch March 5, 2017 22:45
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants