Skip to content
This repository has been archived by the owner on Dec 21, 2021. It is now read-only.

use shared eslint-config-streamr-ts #252

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

harbu
Copy link
Contributor

@harbu harbu commented May 7, 2021

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.
  • There were a few 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.

@harbu harbu requested review from timoxley and teogeb May 7, 2021 14:36
cleanup()
// @ts-expect-error
resolve() // noop
}
Copy link
Contributor

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?

Copy link
Contributor Author

@harbu harbu May 10, 2021

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) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

same thing here

Copy link
Contributor Author

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);
    });
}

Copy link
Contributor Author

@harbu harbu May 10, 2021

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
Copy link
Contributor

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
Copy link
Contributor

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants