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

Custom ESLint rules #2446

Closed
monsieurnebo opened this issue Oct 13, 2017 · 8 comments
Closed

Custom ESLint rules #2446

monsieurnebo opened this issue Oct 13, 2017 · 8 comments
Labels
stale? Issue that may be closed soon due to the original author not responding any more.

Comments

@monsieurnebo
Copy link
Contributor

monsieurnebo commented Oct 13, 2017

Environment

Product Version
Gatsby version 1.9.55
NodeJS 8.5.0
macOS 10.11.6

The problem

Context

Hi folks,

I got a generic ESLint configuration for ReactJS based projects that I'm using on a Gatsby-based project.

ESLint has no clue about Gatsby's webpack existence and fires some false-positives because of three main issues:

  • No extension resolving (e.g. .jsx) causing import/no-unresolved errors
  • No alias resolving (e.g. /helpers/) causing import/no-unresolved errors
  • Loading gatsby-plugin-... version of plugins (e.g. styled-components), but importing original-plugin in the project causing no-extraneous-dependencies errors

gatsby-node.js

const path = require("path");
const pathRoot = (...args) => path.resolve(__dirname, ...args);

exports.modifyWebpackConfig = ({ config, stage }) => {

  config.merge({
    resolve: {
      extensions  : [".js", ".jsx"],
      alias       : {
        config  : pathRoot("config"),
        helpers : pathRoot("src", "helpers"),
        shared  : pathRoot("src", "shared")
      }
    }
  });

  return config;

};

Ugly fix

We can fix the third issue by declaring core-modules like that (not so clean & difficult to maintain):

"import/core-modules": [
      "react",
      "prop-types",
      "react-helmet",
      "styled-components"
    ],

The two other rules are temporary (?) disabled...

--> Is there any clean solution (other than this) ?


Related to

#463
#1990
#362

@jquense
Copy link
Contributor

jquense commented Oct 13, 2017

There are few things I think we can do to mitigate this. I think the first important step is to release an eslint-config-gatsby that sets up all the relevant globals, "core" module, and the like

@monsieurnebo
Copy link
Contributor Author

monsieurnebo commented Oct 17, 2017

@jquense That's a great idea for the import/core-modules part.

But I still don't see any good solution for the disabled rules.

@jquense
Copy link
Contributor

jquense commented Oct 17, 2017

the other's are easy enough to handle with a config i thinl, though i don't quite understand these concerns:

No alias resolving (e.g. /helpers/) causing import/no-unresolved errors
Loading gatsby-plugin-... version of plugins (e.g. styled-components), but importing original-> > plugin in the project causing no-extraneous-dependencies errors

what is /helpers/ and why doesn't importing gatsby-plugins directly work? everything should be listed in the site package.json (and resolvable)

@monsieurnebo
Copy link
Contributor Author

monsieurnebo commented Oct 19, 2017

About gatsby plugins

If I'm right, plugins which have a gatsby version (such as styled-components and react-helmet) should be listed as gatsby-plugin-foo-bar and not foo-bar in the package.json.

For instance, take the official example of Styled Component: it's only using gatsby-plugin-styled-components and not styled-components.

package.json

  "dependencies": {
    "gatsby": "latest",
    "gatsby-plugin-styled-components": "latest",
  },

In a component, you still import the original plugin, and not the gatsby version:

MyComponent.jsx

import styled from "styled-components";

This package is loaded from the gatsby plugin itself which has a styled-components dependency...

... Firing the import/no-extraneous-dependencies ESLing rule:

ESLint

  1:1  error  'styled-components' should be listed in the project's dependencies. Run 'npm i -S styled-components' to add it  import/no-extraneous-dependencies

I can see two solutions for that:

  1. Add the original package dependency in the Gatsby project?
  2. Add a import/core-modules ESLint rule for theses packages?

About Webpack resolving

File extensions

Fixed by the following ESLint rule:

"import/extensions" : ["error", { "jsx": "never" } ]

Aliases

We have several directories in the /src/ which are shared across the project (e.g. /helpers/), but not shared in other projects (that's why they aren't in a package).

In order to use them from any file while avoiding the /../../../helpers/foo.js, we have set a webpack helpers/... alias. In a non-Gatsby React project, ESLint can understand these aliases and resolve them. In a Gatsby project, ESLint isn't aware of the webpack config and isn't able to resolve as a result.

A temporary solution is to disable this ESLint rule, but it's an open door for real unresolved errors:

"import/no-unresolved" : "off",

@jquense Is that more understandable? :)

@jquense
Copy link
Contributor

jquense commented Nov 1, 2017

So i think some of the problem here is that we've chosen a small convenience at the expense of defeating tools and best practices. Frankly I think that Gatsby should not provide these modules without you installing them. Take the styled-components, the correct way to structure that plugin is to have styled-components be a peerDependency of the plugin (like the emotion-plugin), not a direct dependent, if styled-components really only needs one instance of it, otherwise i'd say we shouldn't automagically let you import the plugins dependency. Ditto for packages like React, prop-types, etc. There is just too much issue with this and no good way of letting all the tooling know about, instead the starter should just install these dependencies directly, and then you can treat them like any other dep.

@KyleAMathews
Copy link
Contributor

Yeah… with starters and us giving the npm install instructions in packages — people shouldn't have too much trouble getting the right packages installed.

Perhaps make this switch for v2?

jlengstorf added a commit to jlengstorf/gatsby that referenced this issue Nov 19, 2017
- Remove react-helmet from dependencies
- Add react-helmet from peerDependencies

BREAKING CHANGE: Requires that react-helmet is installed alongside
gatsby-plugin-react-helmet.

re gatsbyjs#2446
KyleAMathews pushed a commit that referenced this issue Nov 20, 2017
- Remove react-helmet from dependencies
- Add react-helmet from peerDependencies

BREAKING CHANGE: Requires that react-helmet is installed alongside
gatsby-plugin-react-helmet.

re #2446
@brandonkal
Copy link

For what it is worth, I created this for linting my own projects: https://www.npmjs.com/package/eslint-config-gatsby-standard

@gatsbot
Copy link

gatsbot bot commented Jan 17, 2019

Old issues will be closed after 30 days of inactivity. This issue has been quiet for 20 days and is being marked as stale. Reply here or add the label "not stale" to keep this issue open!

@gatsbot gatsbot bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale? Issue that may be closed soon due to the original author not responding any more.
Projects
None yet
Development

No branches or pull requests

6 participants