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

Disable gatsby eslint if local found #5871

Merged
merged 2 commits into from
Jun 15, 2018

Conversation

kkemple
Copy link
Contributor

@kkemple kkemple commented Jun 13, 2018

This PR adds a check for local eslint config and if found, disables the eslint check in Gatsby webpack config.

Closes #5763
Closes #5493

// not sure what we should do here...
}

const eslintFiles = glob.sync(`.eslintrc*`, { cwd: directory })
Copy link
Contributor

Choose a reason for hiding this comment

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

this might produce some false positives if someone tries to rename their .eslintrc to something like for example .eslintrc.tmp - I think we should use more strict glob here to find exactly those files that eslint would use:

https://github.com/eslint/eslint/blob/a370da27a7957e2a5e038f75cd97c0951f8c4ddc/lib/config/config-file.js#L46-L53 :

const CONFIG_FILES = [
    ".eslintrc.js",
    ".eslintrc.yaml",
    ".eslintrc.yml",
    ".eslintrc.json",
    ".eslintrc",
    "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.

i didn't even think of that! good call, updating now

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should check up the directory structure...I often have a eslintrc in a root directory above the site...like the gatsby repo, where www would use ../ eslint

Copy link
Contributor

@pieh pieh Jun 13, 2018

Choose a reason for hiding this comment

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

Hmm maybe we could use this https://eslint.org/docs/developer-guide/nodejs-api#clienginegetconfigforfile to figure out if eslint would pickup any config and don't play guessing game here?

---edit seems like I posted too fast, not sure if this one would pick actual configs in files or not
---edit2 maybe it would work reading CLIEngine description:

The primary Node.js API is CLIEngine, which is the underlying utility that runs the ESLint command line interface. This object will read the filesystem for configuration and file information but will not output any results. Instead, it allows you direct access to the important information so you can deal with the output yourself.

Copy link
Contributor

@m-allanson m-allanson Jun 13, 2018

Choose a reason for hiding this comment

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

Hmm yeah. This could be confusing if you have an eslint config way up the directory tree though.

With a config file at:

~/.eslintrc

and a Gatsby site at:

~/my-stuff/dev/client-projects/client-a/gatsby-project

the default linting will be disabled. Maybe that's ok though? An alternative could be to search up the directory tree until you find a .git directory and then stop? But maybe that's trying to be too clever.

Copy link
Contributor

Choose a reason for hiding this comment

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

search up the directory tree until you find a .git directory and then stop?

that is not a bad default i don't think 👍 compared to just checking the gatsby dir, ideally we'd be able to use whatever eslint uses, but the issue there is that is very much dependent on the cwd when calling eslint. Maybe we should use whatever logic editor extensions use? that may just be "check the project root" specfic to like vscode, but maybe it's more clever?

Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

This looks good. We can iterate on it in next PRs to handle setups where .eslint is outside of gatsby directory

@m-allanson
Copy link
Contributor

m-allanson commented Jun 15, 2018

Maybe something like cosmiconfig could be used to search across more paths. Although maybe overkill as we don't need to load the config once we find it.

@m-allanson m-allanson merged commit 8895061 into v2 Jun 15, 2018
@m-allanson m-allanson deleted the topics/disable-eslint-if-local-found branch June 15, 2018 14:52
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

Successfully merging this pull request may close these issues.

4 participants