Skip to content
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

Fix TS types for subscription categories #3162

Merged
merged 1 commit into from
Oct 30, 2019
Merged

Fix TS types for subscription categories #3162

merged 1 commit into from
Oct 30, 2019

Conversation

cgewecke
Copy link
Collaborator

#3159

In web3-eth/src/index.js, syncing, newBlockHeaders, and pendingTransactions do not expect any options param.

The 1.x documentation for these subscriptions is correct.

@Velenir
Copy link
Contributor

Velenir commented Oct 27, 2019

The last overload would still allow for web3.eth.subscribe('pendingTransactions', null, console.log)

You can check here

I propose something along the lines of this

@cgewecke
Copy link
Collaborator Author

@Velenir Ah yes, I see what you're saying. (Thanks so much for looking this over - I'm not a typescript expert.)

The types at @types/web3 mirror the more permissive grammar.

    subscribe(
        type: "syncing",
        callback?: Callback<any>
    ): Promise<Subscribe<any>>;
    subscribe(
        type: "newBlockHeaders",
        callback?: Callback<BlockHeader>
    ): Promise<Subscribe<BlockHeader>>;
    subscribe(
        type: "pendingTransactions",
        callback?: Callback<Transaction>
    ): Promise<Subscribe<Transaction>>;
    subscribe(
        type: "pendingTransactions" | "newBlockHeaders" | "syncing" | "logs",
        options?: Logs,
        callback?: Callback<any>
    ): Promise<Subscribe<any>>;

Can you suggest why they might have written the final overload that way? Am slightly wary of changing the rule without understanding why the existing pattern is in place. Is it just a mistake?

I'm pinging @joshstevens19 for an additional peek at this as well...

@nivida nivida added 1.x 1.0 related issues Types Incorrect or missing types labels Oct 28, 2019
@Velenir
Copy link
Contributor

Velenir commented Oct 28, 2019

Great that you brought up that point. My guess would be that final overload was there to match all possible cases of event type, like:

// we don't know which one it will be
const event: "pendingTransactions" | "newBlockHeaders" | "syncing" | "logs" = ...

// before, when all overloads had `options?`,
// the subscribe signature was virtually the same so the following was possible
// still don't know what event is, but don't care, it will work anyway
web3.eth.subscribe(event, options, console.log)

///practical application:
const events: ("pendingTransactions" | "newBlockHeaders" | "syncing" | "logs")[] = 
['pendingTransactions', 'syncing', 'newBlockHeaders', 'logs']

events.forEach((ev) => eth.subscribe(ev, console.log)} // breaks for 'logs'
// have to actually check event
events.forEach((ev) => ev === 'logs' ? eth.subscribe(ev, {}, console.log) : eth.subscribe(ev, console.log)}

But now we know that eth.subscribe(ev, console.log) would break for 'logs' and eth.subscribe(ev, {}, console.log) would break for everything else.

So we need an overload for all events except for 'logs'
I took another stab at it, see here

TS is hard, but that was interesting

@joshstevens19
Copy link
Contributor

joshstevens19 commented Oct 28, 2019

Hey so the last overload should not be there. It is a very pointless type and will confuse TS completely let's kill it. From looking at it the types are all correct we should just remove that last overload and it will work fine and fix that issue for people on TS.

   subscribe(
        type: 'logs',
        options?: LogsOptions,
        callback?: (error: Error, log: Log) => void
    ): Subscription<Log>;
    subscribe(
        type: 'syncing',
        callback?: (error: Error, result: Syncing) => void
    ): Subscription<Syncing>;
    subscribe(
        type: 'newBlockHeaders',
        callback?: (error: Error, blockHeader: BlockHeader) => void
    ): Subscription<BlockHeader>;
    subscribe(
        type: 'pendingTransactions',
        callback?: (error: Error, transactionHash: string) => void
    ): Subscription<string>;

one thing i did see if i'm not sure options are optional in log from the documentation so i think it should probably be:

   subscribe(
        type: 'logs',
        options: LogsOptions,
        callback?: (error: Error, log: Log) => void
    ): Subscription<Log>;

👍

@@ -83,17 +83,14 @@ export class Eth {
): Subscription<Log>;
Copy link
Contributor

Choose a reason for hiding this comment

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

just murder the last overload on like 96 and should be all good to merge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @joshstevens19!

@coveralls
Copy link

coveralls commented Oct 28, 2019

Coverage Status

Coverage remained the same at 84.005% when pulling 93fe751 on cgewecke:issue/3159 into 9dd09db on ethereum:1.x.

Copy link
Contributor

@joshstevens19 joshstevens19 left a comment

Choose a reason for hiding this comment

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

LGTM - we need to do this in the 2.0 branch as well if there’s any point, as there probably will not be anymore 2.x releases until next web3 completely. 🧐

@cgewecke
Copy link
Collaborator Author

@Velenir Am following your original proposal & Josh's recs to remove the overloading altogether and make the LogsOptions non-optional. LGTM with respect to docs & code.

Thanks again, super helpful.

@cgewecke cgewecke requested a review from nivida October 28, 2019 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Types Incorrect or missing types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants