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

Misleading types for EnsProvider.getResolver() #1850

Closed
geigerzaehler opened this issue Aug 9, 2021 · 5 comments
Closed

Misleading types for EnsProvider.getResolver() #1850

geigerzaehler opened this issue Aug 9, 2021 · 5 comments
Labels
minor-bump Planned for the next minor version bump.

Comments

@geigerzaehler
Copy link

The getResolver() method on the EnsProvider interface is declared with the following signature

export interface EnsProvider {
    getResolver(name: string): Promise<EnsResolver | null>;
}

The implementation of getResolver() can return null though

async getResolver(name: string): Promise<Resolver> {
try {
const address = await this._getResolver(name);
if (address == null) { return null; }
return new Resolver(this, address, name);
} catch (error) {
if (error.code === Logger.errors.CALL_EXCEPTION) { return null; }
return null;
}
}

This leads to callers of getResolver() not anticipating null as the return value which leads to runtime errors even though type-checking passes. I’d expect the following signature.

export interface EnsProvider {
    getResolver(name: string): Promise<EnsResolver | null>;
}
@geigerzaehler geigerzaehler added the investigate Under investigation and may be a bug. label Aug 9, 2021
@ricmoo
Copy link
Member

ricmoo commented Aug 9, 2021

What is the difference in those signatures though? (I’m on my phone, so maybe if I cut and paste on a compute it would be more obvious?)

@ricmoo
Copy link
Member

ricmoo commented Aug 9, 2021

Or do you mean you want the implementation to include null in its Promise return type?

I can add that in the next minor version, but the library in general doesn’t have strictNullCheck enabled. This will change in v6 though, so in v6 the return type would enforce Promise<null | EnsResolver>.

@geigerzaehler
Copy link
Author

Or do you mean you want the implementation to include null in its Promise return type?

Yes. But I think the EnsProvider interface would need to change too because this is what is used to determine the types when calling getResolver(). Basically I want typescript to tell me that resolver can be null if I have the following code:

const resolver = await provider.getResolver()

I can add that in the next minor version, but the library in general doesn’t have strictNullCheck enabled. This will change in v6 though, so in v6 the return type would enforce Promise<null | EnsResolver>.

That’s great and it would solve the problem!

@ricmoo ricmoo added minor-bump Planned for the next minor version bump. and removed investigate Under investigation and may be a bug. labels Aug 27, 2021
@ricmoo
Copy link
Member

ricmoo commented Oct 4, 2021

I'm adding this locally for the next minor bump now. :)

Keep in mind a large portion of the library will still have the same problem if you are. "anticipating" things to not be null. The library is overall designed around the strict error in the TypeScript checker turned off, so anything can return null.

This is changed in v6 (which uses strict errors), and has required a lot of little changes, and v6 is still not ready for a public beta yet. But for the most part in v5, these sorts of changes will not be made anymore...

@ricmoo
Copy link
Member

ricmoo commented Oct 20, 2021

The types have been updated in 5.5.0.

Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor-bump Planned for the next minor version bump.
Projects
None yet
Development

No branches or pull requests

2 participants