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
Update dependencies and lint #2391
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @sindresorhus on these notes
@@ -105,13 +105,17 @@ | |||
"rules": { | |||
"no-alert": "off", | |||
"import/no-unassigned-import": "off", | |||
"import/no-unresolved": "warn" | |||
"import/no-unresolved": "warn", | |||
"import/named": "warn" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That rule has other problems too. Disabled: xojs/xo@ee145cb
}, | ||
"overrides": [ | ||
{ | ||
"files": "**/*.+(ts|tsx)", | ||
"extends": "xo-typescript", | ||
"rules": { | ||
"@typescript-eslint/no-misused-promises": "off", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annoying: It doesn't let us use async
functions as event handlers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's convenient, my reasoning is that using async there makes it hard to see that the event handler function will cause an uncaught rejection if the await
rejects. For example, using async in a new Promise
can cause hard to track down issues.
And when people see:
[1, 2, 3].forEach(async value => {
await doSomething(value);
});
they might be fooled into thinking that it awaits for each iteration, which it does not.
I'm happy to discuss this though. Maybe there some additional config that could be added to the rule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the value in that case, but I don't have a solution for our use-case: our handlers are async
just because Promise
handling is great, not because anyone expects addEventListener
to await
them.
If we enable this rule, we have to write listeners as:
function handler() {
(async () => {
document.body.append(await fetchDom(...));
})();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more useful related rule would be the opposite: always await async functions. Does this exist already?
async function set(){...}
async function get(){...}
set(1); // Forgot `await`
await get();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more useful related rule would be the opposite: always await async functions. Does this exist already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost, returnsPromise()
is not awaited even though it returns a promise. It seems that it requires await
only on variables
package.json
Outdated
}, | ||
"overrides": [ | ||
{ | ||
"files": "**/*.+(ts|tsx)", | ||
"extends": "xo-typescript", | ||
"rules": { | ||
"@typescript-eslint/no-misused-promises": "off", | ||
"@typescript-eslint/require-await": "off", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Buggy: it doesn't recognize returning a promise in arrow functions (Edit: will be fixed in the next release). It's already on top of some arguably annoying rules and it conflicts with them:
e.g.
() => Promise.resolve()
✖ Functions that return promises must be async. @typescript-eslint/promise-function-async
async () => Promise.resolve()
✖ Async arrow function has no await expression. @typescript-eslint/require-await
async () => await Promise.resolve()
✖ Redundant use of await on a return value. no-return-await
The first one was perfectly fine (EDIT:) and for this reason, I disabled the initial rule (2036c3e) since require-await
seems to be more useful.
}, | ||
"overrides": [ | ||
{ | ||
"files": "**/*.+(ts|tsx)", | ||
"extends": "xo-typescript", | ||
"rules": { | ||
"@typescript-eslint/no-misused-promises": "off", | ||
"@typescript-eslint/require-await": "off", | ||
"@typescript-eslint/no-explicit-any": "off", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good in theory, but makes us have to type one-off things just for the heck of it:
-
Code examples:
(window as any).select = select;
const editor: CodeMirrorInstance = document.querySelector<any>('.CodeMirror').CodeMirror;
(global as any).navigator = window.navigator; (global as any).document = window.document; (global as any).location = new URL('https://github.com'); // Which becomes: (global as unknown as typeof window).navigator = window.navigator; (global as unknown as typeof window).document = window.document; (global as unknown as typeof window).location = new URL('https://github.com');
-
It'd force us to type API calls
-
Also kills
AnyObject
which is pretty much required for the API handler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was that you explicitly disable the rule where you want any
to make it inconvenient enough to use that it's not misused, but I encountered some annoyance with it too, so I'll disable it for now: xojs/eslint-config-xo-typescript@bbbf889
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was that you explicitly disable the rule where you want
any
to make it inconvenient enough to use that it's not misused
Maybe that is worth it, I missed this one just recently: https://github.com/sindresorhus/refined-github/pull/2393/files#diff-f8fed5d366f31fce028a9c8ceca38c3eR6
The lines in my examples would just have to use eslint-disable-next-line
or be wrapped by disabled/enable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be easier to enable in a year or so when TS has matured more and we need less any
in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What other problems have you had?
Do you think there will ever be a solution to my examples? They basically are:
- setting a global (without having to create a property on the
Global
/Window
types) - setting/getting one-off properties. It could probably already be fixed with:
type CodeMirrorElement = Element & {CodeMirror: CodeMirrorInstance} const editor = document.querySelector<CodeMirrorElement>('.CodeMirror').CodeMirror;
- we just want to avoid typing of certain parts like APIs because it's extra work that has to be updated when the query changes, otherwise the API response won't actually match the types (in essence, manually typing the API is as bad as an
as
sertion). Also the API response ALWAYS has to start fromAnyObject
, there's no workaround for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had problems with some bad types from Electron and React. Basically out of my control. I also had to use any
for some things like this: https://github.com/sindresorhus/caprine/blob/7965b36c11ae1f7ccb2185bebdc85e9fc3d01078/source/menu.ts#L92
In the future, there will be less globals, and the types of large projects will be better, TS will be better.
@@ -1,3 +1,5 @@ | |||
// TODO: Drop some definitions when their related bugs are resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I merged all of these TODOs into one. They're just noise since we can only wait for the underlying bugs to be fixed.
"@typescript-eslint/parser": "^1.11.0", | ||
"ava": "^2.2.0", | ||
"@typescript-eslint/eslint-plugin": "^1.13.0", | ||
"@typescript-eslint/parser": "^1.13.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm temporarily avoiding v2 because of a bug I don't know how to work around: typescript-eslint/typescript-eslint#890
webpack.config.ts:undefined:undefined
✖ 0:0 Parsing error: If "parserOptions.project" has been set for @typescript-eslint/parser, ./webpack.config.ts must be included in at least one of the projects provided.test/page-detect.ts:undefined:undefined
✖ 0:0 Parsing error: If "parserOptions.project" has been set for @typescript-eslint/parser, ./test/page-detect.ts must be included in at least one of the projects provided.test/utils.ts:undefined:undefined
✖ 0:0 Parsing error: If "parserOptions.project" has been set for @typescript-eslint/parser, ./test/utils.ts must be included in at least one of the projects provided.test/fixtures/globals.ts:undefined:undefined
✖ 0:0 Parsing error: If "parserOptions.project" has been set for @typescript-eslint/parser, ./test/fixtures/globals.ts must be included in at least one of the projects provided.
They should go away by adding test
in tsconfig's includes
, but that file appears to be ignored.
Also reported in xojs/eslint-config-xo-typescript#15
No description provided.