Ethers.js: Type for getTransaction misses `| null`

[roderik]

Ok, so first of all, I found this due to a programming error so probably not an issue that happens for a lot of people, but types could have caught this.

• provider.getTransaction returns Promise<TransactionResponse>
• but call this function with a transaction has from e.g. a different chain or a dropped transaction, JSON RPC returns null
• Ethers (v5) passes along this null while the types think it is a TransactionResponse and do not complain about accessing tx.data for example

Example call in JSON RPC which triggers this in ethers:

curl -X POST --data '{"jsonrpc":"2.0","method":"eth_getTransactionByHash","params":["0xa52be92809541220ee0aaaede6047d9a6c5d0cd96a517c854d944ee70a0ebb44"],"id":53}' -H "Content-Type: application/json" https://mintnet.settlemint.com

{"jsonrpc":"2.0","result":null,"id":53}

Maybe this is client/version specific so for reference this is a Parity Aura network running 2.5.5

[roderik]

ps. | null checks for everything is also a pain, an exception could also handle this

[zemse]

You're correct that getTransaction would return null if tx doesn't exist.

providerETH.getTransaction('0xa52be92809541220ee0aaaede6047d9a6c5d0cd96a517c854d944ee70a0ebb44').then(console.log)
Promise {<pending>}
null

IMO the getTransaction should have | null return type, since the purpose of using TypeScript in projects is to catch errors before deploying and avoid bugs during runtime. Having such a return type would remind the developer to handle a null return value to avoid a possible runtime failure, or they can simple have // @ts-ignore above the line.

Bad thing about this that this would definitely cause existing ts code bases on ethers v5 (including plenty of mine) to break and cause headache to devs. However, it's upto @ricmoo if this will be changed. Is this change possible in the next minor version (5.1.x)? Since there is no API change for javascript devs.

[ricmoo]

Changing to strict null checking will not be made in the v5 branch as @zemse said the breakage would be significant.

In v6 this change will be made, and will be a fairly involved process, but I agree it is very useful, especially in the context of a library.

I could consider adding | null to the definition. I don’t think that could cause anything to break? It might be a good start toward strict null checks too...

[roderik]

This is not even strict null checking, this is returning data that does not adhere to the typescript types. There is no way to compile-time check what data might be returned from the JSON RPC API.

Unfortunately adding | null will break all typescript uses of getTransaction, since everything then needs a check if it is not null.
I do not see a BC way to fix this.

Depending on your definition you could either argue that correctness trumps required developer changes before compile, all logic stays the same, it will work the same, just types are a bit different, or it means that you can upgrade without any developer work needed.

I'm straying more to the latter than the former. In projects with fast major version updates (think ipfs at v45 or whatever) I would argue to just push out v6, but ethers has a more conservative schedule.

Throwing an exception with a clear message might be a good intermediary solution. It does not change the type and will alert a dev that something is wrong. Only code that does check for null (as it should) and expects this when checking non-existing tx hashes will have to add a try catch.

[garyng2000]

given that most return are sort of 'nullable type'(or MayBe in the Haskell context which is just another form of saying null), what is the problem return null as it is now ?

[ricmoo]

Great question for TypeScript people. Would love some feedback.

Will adding | null to the return type cause the type checker to explode? Is there any danger changing number to number | null for example? Or is it as @garyng2000 suggested, that number would already be described as nullable, so already be iplicitly defined as number | null?

I can do some experimentation, but would love to get one of the TypeScript guru's feedback too. :)

I may just punt this until v6 regardless. Moving to strict null checks is already on the v6 roadmap, so this will just happen as part of that bullet point. Backwards compatibility is among the most important things to me. :)

[ricmoo]

I think I've decided to punt until v6, where the strict null check will force this. I'm afraid of breaking existing code by making that change to the signature.

I'll close for now, but if someone has a deeper understanding of the TS type checker (across all versions from 3.8 and above), please feel free to re-open or chime in.

Thanks! :)

[phiresky]

Unfortunately adding | null will break all typescript uses of getTransaction, since everything then needs a check if it is not null.

Code that uses getTransaction has to check for null since it will otherwise crash for dropped transactions. Existing code that does not do this is wrong, I was just bitten by this. Correcting the TypeScript types is not a breaking change, it's a bug fix