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

feat: remove unused type guard typings #550

Merged
merged 1 commit into from
Aug 5, 2022

Conversation

gomesalexandre
Copy link
Contributor

Description

This removes the unused type guard (x is Y) from the supportsXYZ methods

Note that the supportsCosmos and supportsOsmosis ones are kept, as they are actually used in chain-adapters.

Tried linking in lib and web and this builds/passes type-check.

Issue

@vercel
Copy link

vercel bot commented Jul 13, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
hdwallet ✅ Ready (Inspect) Visit Preview Jul 13, 2022 at 0:31AM (UTC)

@gomesalexandre
Copy link
Contributor Author

@pastaghost Keeping this as a draft waiting for your input here. In the context of our consumptions, the type guards are effectively useless, but now that I'm thinking about it, we might have other projects consuming this and it might bring breaking type changes for them.

Happy to close both this and the issue with the rationale that hdwallet is open-source and we shouldn't tie implementations to our own usages.

@0xApotheosis
Copy link
Member

@pastaghost thoughts on this one re: @gomesalexandre comment?

I'm in favour of removing the type guard typings, as these functions aren't actually type guards (beyond their syntax), and so are misleading.

Copy link
Member

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

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

I think this PR makes these functions more correct, as they are more utility functions than actual type guards.

Pending thoughts from @pastaghost.

@pastaghost
Copy link
Collaborator

I agree with @0xApotheosis on this one - I don't think this is proper use of a user-defined type guard. I'm in favor of removing the type guard returns and have approved the PR.

@gomesalexandre gomesalexandre merged commit 27a4fd3 into master Aug 5, 2022
@mrnerdhair
Copy link
Contributor

IMO this doesn't make sense. These type guards suck, but there's no reason to erase the metadata about what they do -- especially if some are still in use. (It wasn't that long ago when we were using supportsETH in web/lib.)

I recommend either keeping them or removing them completely, along with the _supportsFoo stuff they inspect, and switching over to using instanceof consistently.

@mrnerdhair
Copy link
Contributor

(For an example of the above, see #435, which was closed at the time due to lack of review bandwidth.)

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

Successfully merging this pull request may close these issues.

Remove unused type guards
4 participants