ref(eslint): Convert all packages to use central eslint config #4111
Conversation
size-limit report
|
lobsterkatie
left a comment
lobsterkatie
left a comment
There was a problem hiding this comment.
A few small things but generally looks good!
| // eslint-disable-next-line no-prototype-builtins | ||
| if (this.options.headers.hasOwnProperty(header)) { |
There was a problem hiding this comment.
Any reason not to just fix this rather than ignore it?
| // eslint-disable-next-line no-prototype-builtins | |
| if (this.options.headers.hasOwnProperty(header)) { | |
| if (Object.prototype.hasOwnProperty.call(this, header)) { |
There was a problem hiding this comment.
Wanted to make the code change as minimal as possible. I'll come back and address these low hanging fruits in separate PRs.
| '@typescript-eslint/explicit-member-accessibility': 'off', | ||
| '@typescript-eslint/no-explicit-any': 'off', | ||
| '@typescript-eslint/no-non-null-assertion': 'off', | ||
| '@typescript-eslint/no-empty-function': 'off', |
There was a problem hiding this comment.
I'm curious - why turn no-empty-function off?
There was a problem hiding this comment.
Feel like it's not valuable for tests, where you might want to quickly add empty functions if you are stubbing out a class/object.
| extends: ['../../.eslintrc.js'], | ||
| rules: { | ||
| 'max-lines': 'off', | ||
| '@sentry-internal/sdk/no-async-await': 'off', |
There was a problem hiding this comment.
Do we need this if this is the default?
There was a problem hiding this comment.
The default is error (turned on). We turn it off for tests.
| sentryLevel = Severity.Log; | ||
| } | ||
|
|
||
| /* eslint-disable prefer-rest-params */ |
There was a problem hiding this comment.
It's not letting me do this as a suggestion (I guess because it spans a part which it hides by default), but since it's an easy fix IMHO we should just change arguments to ...args and not ignore the rule.
There was a problem hiding this comment.
Will circle back to this in a PR afterwards to not increase the scope of this PR
Convert nextjs, react, node, and serverless to using the central eslint config. This should make sure all packages use a consistent eslint config. There are some exceptions which I will walk through.
Note: Ember is not converted as it's special cased (part of the build) - so I kept it separate. We can have a discussion about what we wanna do with ember, but my opinion is that it's fine to keep it a special case.
In our central eslint config, I've disabled our
no-async-awaitrule in tests and made jsdoc required for only exported methods/classes.For browser we've kept
ignorePatterns: ['test/integration/**', 'src/loader.js'],. I think it's fine to not lint the loader, and the integration tests are going to be refactored with the release stability work, so we can keep this for now.