-
Notifications
You must be signed in to change notification settings - Fork 9
use shared eslint-config-streamr-ts #252
base: master
Are you sure you want to change the base?
Conversation
cleanup() | ||
// @ts-expect-error | ||
resolve() // noop | ||
} |
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.
Previously this let + late declaration was required because it'd complain that onDisconnected
was being used before it was defined, probably supposed to detect temporal deadzone e.g. in connection.off('disconnected', onDisconnected)
. I guess something just got smarter about this rather than the use-before-define detection or whatever being disabled?
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.
To be honest I was a bit uneasy changing these, but assumed they should work. The previous version complained about using let:
/home/harbu/work/streamr-client-javascript/src/stream/utils.ts
113:13 error 'onDisconnected' is never reassigned. Use 'const' instead prefer-const
However it does feel a bit off referencing a symbol that only gets defined later on...
const onEvent = (value: any) => { | ||
emitter.off('error', onError) | ||
resolve(value) | ||
} | ||
onError = (error) => { | ||
const onError: (error: Error) => void = (error) => { |
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.
same thing here
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.
Well I checked and seems like TypeScript doesn't work any magic around this, the compiled output is
function waitFor(emitter, event) {
return new Promise((resolve, reject) => {
const onEvent = (value) => {
emitter.off('error', onError);
resolve(value);
};
const onError = (error) => {
emitter.off(event, onEvent);
reject(error);
};
emitter.once(event, onEvent);
emitter.once('error', onError);
});
}
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.
Ah, we might wanna re-enable rule https://eslint.org/docs/rules/no-use-before-define @timoxley and revert these changes , agree?
const onDisconnected = () => { | ||
cleanup() | ||
// @ts-expect-error | ||
resolve() // noop |
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.
This // @ts-expect-error
will probably go away with a resolve(undefined)
@@ -8,7 +8,9 @@ import type { StreamPartDefinitionOptions } from '../src/stream' | |||
import { StreamrClient } from '../src/StreamrClient' | |||
import { PublishRequest } from 'streamr-client-protocol/dist/src/protocol/control_layer' | |||
|
|||
// eslint-disable-next-line @typescript-eslint/no-var-requires |
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.
These can actually probably be regular import
calls.
a4a4dfb
to
411d79d
Compare
Use shared eslint-config-stream-ts in line with streamr-dev/network#555. Had to add in a few additional rules to accommodate ongoing transition into TypeScript. Particularly rules:
@typescript-eslint/ban-ts-comment
many instances of@ts-expect-error
without description. Not worth adding in descriptions imo since they will be most likely replaced with types later on.no-underscore-dangle
again a lot of private fields leftover from javascript days, prolly worth changing when working on said code to prevent unexpected changes in behavior.require
statements left. They were tactically placed, so I assumed they are most likely there on purpose. Annotated such with the following// eslint-disable-next-line import/no-unresolved
.