Fix most "systematic" lint issues #995
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Good news and bad news:
The good news is that the linter is no longer choking on stuff in our
JSX files.
The bad news is that our error count has gone up as a result:
Before:
✖ 436 problems (272 errors, 164 warnings)
After:
✖ 926 problems (691 errors, 235 warnings)
I'll continue whittling away the warnings in subsequent commits.
These are files we couldn't parse until the previous commit added
babel-eslint.
Before:
✖ 926 problems (691 errors, 235 warnings)
After:
✖ 778 problems (543 errors, 235 warnings)
We have a bunch of lints like:
A constructor name should not start with a lowercase letter new-cap
Caused because of patterns like:
new CKEDITOR.dom.walker(...);
This commit sets up exceptions for those. Note that I listed the
offenders explicitly instead of using a pattern because I didn't want
typos like "CKEDITOR.dom.wakler(...)" to slip through.
Before:
✖ 778 problems (543 errors, 235 warnings)
After:
✖ 688 problems (453 errors, 235 warnings)
To make it at least somewhat discoverable. You can already achieve the same with `npm run lint -- --quiet`, but this makes the availability of this functionality more obvious to anybody reading the package.json file. This is important because we currently have 235 warnings (related to JSDoc) which, realistically, we won't be able to fix for a long time. Bonus: moved `--fix` to right after the executable name in the neighboring "fix" task for consistency.
These are our HOC wrapper functions that all start with capital letters
even though they are not called with `new`. We may remove some (or all)
of these in favor of other abstractions, so rather than change their
names, I am suppressing the lints related to these functions.
Fixed a couple of capitalization inconsistencies along the way as a
result of doing this lint:
- ButtonKeyStroke -> ButtonKeystroke (to match other usages)
- columnResizer -> ColumnResizer (because it really is a constructor)
Before:
✖ 688 problems (453 errors, 235 warnings)
After:
✖ 572 problems (337 errors, 235 warnings)
ESLint will get confused without this plug-in, for example, thinking
that an import is unused if it appears as a JSX component.
So, we add the plug-in and that takes us from:
✖ 572 problems (337 errors, 235 warnings)
to:
✖ 305 problems (305 errors, 0 warnings)
Only cheating slightly here by turning "react/prop-types" down from
"error" to "warn". Without this the error count skyrockets and the whole
thing becomes unworkable; making these warnings means that we can chew
through them slowly over time.
This takes us down from:
✖ 305 problems (305 errors, 0 warnings)
to:
✖ 298 problems (298 errors, 0 warnings)
and additionally gets rid of this console noise at the beginning of the
run:
Warning: React version not specified
in eslint-plugin-react settings. See
https://github.com/yannickcr/eslint-plugin-react#configuration .
I haven't been running the "format" task on this file as I edit it; I will set it up to run automatically in the future.
Fixes these lints: ``` 71:43 error Unnecessary escape character: \/ no-useless-escape 75:35 error Unnecessary escape character: \? no-useless-escape 80:3 error Expected an object to be thrown no-throw-literal 199:4 error Unexpected 'this' no-invalid-this 260:7 error 'regexBasePath' is assigned a value but never used no-unused-vars 260:32 error Unnecessary escape character: \/ no-useless-escape ``` The bug: `regexBasePath` was indeed unused, because it wasn't exported in the refactor done in bdbd6b0. This is all a bit magical, but here is how it works: Our webpack build uses "adapter/main" as an entry point with configuration `library: 'AlloyEditor'` and `libraryTarget: 'window'`; this has the effect of exposing everything exported by the adapter as properties of `window.AlloyEditor` (but note, not `regexBasePath` because it wasn't exported). Another webpack build target uses "scripts/build/index" as an entry point, which in turn imports `*` from the adapter and exposes it as under `window.AlloyEditor`. So that's how the exports of the build magically makes the adapter's exports available on `AlloyEditor`, so that calls like `AlloyEditor.[something]` can work. But as I said above, this is *not* happening for `regexBasePath` even though the adapter itself uses it (`getBasePath` is called by `getUrl` which is called by `loadLanguageResources`). Why isn't this blowing up in the release? Two reasons: 1. In the open source release when we hit the line that does `match(AlloyEditor.regexBasePath)`, that's really equivalent to `match(undefined)`, which doesn't blow up and instead just matches against the beginning of the string. There is more code below this that I didn't analyze: maybe it gets through somehow; or maybe it blows up when we throw an error ("installation path could not be automatically detected"). The error instructs people how to work around the error anyway (by setting the `ALLOYEDITOR_BASEPATH` global variable), so perhaps that is how people are dealing with it. 2. In Liferay Portal, we overwrite `AlloyEditor.regexBasePath` at runtime because our built bundle has a different name (ie. with a "liferay" prefix), and so the default regex wouldn't work for us. The fix, then, is to export `regexBasePath`. At this point, src/adapter/main.js is lint-free.
This whole thing is fragile, but the truth is in the codebase we only ever call `AlloyEditor.[something}` so this is safe.
Without this plugin, ESLint spuriously warns about a bunch of legitimate
stuff, such as the use of `this` arrow functions in class instance
methods:
```
class extends WrappedComponent {
applyStyle = () => {
if (this.isActive()) { // <-- complains about "no-invalid-this"
...
```
So we turn *on* the "babel/no-invalid-this" rule and turn *off* the
built-in "no-invalid-this".
Before:
✖ 292 problems (292 errors, 0 warnings)
After:
✖ 165 problems (165 errors, 0 warnings)
The lint: 29:29 error 'nextProps' is defined but never used no-unused-vars We were calling `super()` without forwarding the argument. Test plan: `npm run test` and also `npm run dev && npm run start` and manually test demo.
julien
approved these changes
Jan 25, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These are the ones where I was mostly able to fix things by tweaking the config. After this I'll move on to the next tranche (which will be ones that require edits, but at least form a consistent pattern).