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

chore: update eslint to fix linting issues #29988

Merged
merged 10 commits into from Mar 4, 2021
Merged

chore: update eslint to fix linting issues #29988

merged 10 commits into from Mar 4, 2021

Conversation

wardpeet
Copy link
Contributor

@wardpeet wardpeet commented Mar 4, 2021

Description

Upgraded eslint to latest and fixed rules along the way.

Documentation

Related Issues

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Mar 4, 2021
Comment on lines +5 to +7
// if (process.env.NODE_ENV !== `test`) {
// ignore.push(`**/__tests__`)
// }
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 breaks linting as babel will return an empty ast. Packages should be build with babel src --out-dir . --ignore \"**/__tests__\"

Comment on lines -11 to -12
"prettier/flowtype",
"prettier/react",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are now inside prettier

],
plugins: ["flowtype", "prettier", "react", "filenames"],
plugins: [`flowtype`, `prettier`, `react`, `filenames`, `@babel`],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added @babel to augment the this rules

Comment on lines +76 to +78
"prefer-promise-reject-errors": `warn`,
"no-prototype-builtins": `warn`,
"guard-for-in": `warn`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are new rules and moved them to warnings because there were too many.

.eslintrc.js Outdated
"prefer-promise-reject-errors": `warn`,
"no-prototype-builtins": `warn`,
"guard-for-in": `warn`,
"spaced-comment": [`error`, `always`, { markers: [`/`] }],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

enable typescript /// referrence comments

Comment on lines +80 to +87
camelcase: [
`error`,
{
properties: `never`,
ignoreDestructuring: true,
allow: [`^unstable_`],
},
],
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 have no idea why this wasn't necessary before 🤷

@@ -1,4 +1,4 @@
/* global __PATH_PREFIX__ CMS_PUBLIC_PATH */
/* global CMS_PUBLIC_PATH */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PATH_PREFIX is in the global globals list

Comment on lines -50 to -52
if (node.url.indexOf(`images.ctfassets.net`) === -1) {
return resolve()
}
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 really weird and I'm astonished that this hasn't lead to any bugs.

@@ -71,12 +71,12 @@ const doesConfigChangeRequireRestart = (
const getDebugPort = (port?: number): number => port ?? 9229

export const getDebugInfo = (program: IProgram): IDebugInfo | null => {
if (program.hasOwnProperty(`inspect`)) {
if (Object.prototype.hasOwnProperty.call(program, `inspect`)) {
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 was fixing this rule but eventually moved to warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://eslint.org/docs/rules/no-prototype-builtins

It's a security risk as technically program could have overwritten hasOwnProperty

Copy link
Contributor

Choose a reason for hiding this comment

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

Neat. I'd not thought of that

@wardpeet wardpeet added type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Mar 4, 2021
@@ -784,7 +784,7 @@ export const createWebpackUtils = (
],
...eslintConfig(schema, jsxRuntimeExists),
}
//@ts-ignore
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Now I'm wondering why our ts rules are allowing this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have a lint rule for the space in typescript?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, but we do have a rule that requires a description when there's a ts-ignore comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only a warning :)

        "@typescript-eslint/ban-ts-comment": [
          `warn`,
          { "ts-ignore": `allow-with-description` },
        ],

Co-authored-by: Matt Kane <matt@gatsbyjs.com>
@wardpeet wardpeet marked this pull request as ready for review March 4, 2021 13:41
ascorbic
ascorbic previously approved these changes Mar 4, 2021
@wardpeet wardpeet merged commit 5636389 into master Mar 4, 2021
@wardpeet wardpeet deleted the fix/eslint branch March 4, 2021 18:26
mwfrost pushed a commit to mwfrost/gatsby that referenced this pull request Apr 20, 2023
* chore: update eslint to fix linting issues

* cleanup

* remove /

* Update packages/gatsby/src/commands/serve.ts

Co-authored-by: Matt Kane <matt@gatsbyjs.com>

* update config

* update circle to keep babel-preset-gatsby-package so we can use it in
our babel config

* fix unit tests

* fix snapshot

* fix thigns

* update gatsby-node

Co-authored-by: Matt Kane <matt@gatsbyjs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants