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

ESLint: Issues highlighted in default airbnb styleguide valid? #463

Closed
ryanswrt opened this issue Sep 25, 2016 · 8 comments
Closed

ESLint: Issues highlighted in default airbnb styleguide valid? #463

ryanswrt opened this issue Sep 25, 2016 · 8 comments

Comments

@ryanswrt
Copy link
Contributor

Some of the React best practice warnings and import warnings highlight reasonable issues in the code; is there a reason these aren't being followed? Especially regarding the module imports; as having a resolvable/valid package.json file could help in portability between gatsby and a plain npm react project.

@KyleAMathews
Copy link
Contributor

You're welcome to add PRs fixing this. TBH, I haven't gone through the list of changes in a while so I'm not entirely sure why some of the exceptions were made originally. It's on my mental list of TODOs for the 1.0 release but isn't something I'm going to get to soon and perhaps not for the 0.x line.

@ryanswrt
Copy link
Contributor Author

Ok cool, I'll see what low-hanging errors I can find and fix, if there isn't an underlying reason for it. I think the unresolved gatsby-helpers module can't be solved as it's not on NPM & imports not being resolved due to missing extensions might be tricky due to webpack? Will look into it.

@KyleAMathews
Copy link
Contributor

That particular problem will be fixed in 1.0 as we'll be moving Gatsby to a Lerna monorepo so gatsby-helpers will be published separately to Github.

@andrewagain
Copy link
Contributor

andrewagain commented May 26, 2017

I was initially annoyed when I got the import/no-extraneous-dependencies ESLint error for React. Gatsby seems to provide React via Webpack somehow, but ESlint isn't aware of it.

Then I figured out that you can configure ESLint to consider certain packages as 'core' to get rid of this warning. My .eslintrc.js:

  ...
  settings: {
    // These packages are provided 'magically' by Gatsby
    'import/core-modules': ['react', 'config'],
  },
  rules: {
    ...

Core modules docs: https://github.com/benmosher/eslint-plugin-import#importcore-modules

@kripod
Copy link
Contributor

kripod commented Sep 1, 2017

I'm not sure whether it's appropriate, but I think that peerDependencies should be used for including e.g. react, prop-types and react-helmet. It would make handling dependencies more transparent.

@kripod
Copy link
Contributor

kripod commented Nov 29, 2017

@KyleAMathews Why did you close this? I think that using peerDependencies could eliminate a lot of warnings for yarn users.

@KyleAMathews
Copy link
Contributor

@kripod we're working on that already (gatsby-plugin-react-helmet & gatsby-plugin-styled-components have been upgraded) and #1990 is a much more focused issue for this effort. Sorry for not explaining :-)

@kripod
Copy link
Contributor

kripod commented Nov 29, 2017

@KyleAMathews Fantastic! I'm amused by your quick and accurate responses, keep up the great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants