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.

Sign up for GitHub

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

Jump to bottom

Update window.ethereum type to unknown #432

Merged
merged 4 commits into from
Mar 25, 2022
Merged

Update window.ethereum type to unknown #432

merged 4 commits into from
Mar 25, 2022

Conversation

erin-at-work
Copy link
Contributor

@erin-at-work erin-at-work commented Mar 25, 2022 •
edited
Loading

Summary

  • Prevent conflicting wallet providers for window.ethereum

erin-at-work
erin-at-work commented Mar 25, 2022
View reviewed changes
src/index.ts Outdated Show resolved Hide resolved
@erin-at-work erin-at-work changed the title Update window.ethereum type to any Update window.ethereum type to unknown Mar 25, 2022
ljharb
ljharb reviewed Mar 25, 2022
View reviewed changes
src/index.ts
* Set as CoinbaseWalletProvider
*/
ethereum?: any;
ethereum?: unknown;
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if we could instead declare unknown | PreciseType so that type hinting is more useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had first attempted that, but it doesn't seem to get picked up.

// In a union an unknown absorbs everything

from microsoft/TypeScript#24439

image

and coinbaseWalletExtension:
image

I'll add back a comment

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, oof then.

Copy link
Contributor Author

@erin-at-work erin-at-work Mar 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@ljharb Thoughts on addressing this issue? #343

All declarations of 'ethereum' must have identical modifiers

In order to fix the above error, the type would need to be updated to:

interface Window {
  ethereum: any | undefined;
}

But I hesitate to make changes based on a library's conflicting type defs

@erin-at-work erin-at-work marked this pull request as ready for review March 25, 2022 18:50
@cb-heimdall
Copy link
Collaborator

Review Error for bangtoven @ 2022-03-25 19:27:40 UTC
User failed mfa authentication, see go/mfa-help

@erin-at-work erin-at-work merged commit d82aeb3 into master Mar 25, 2022
@erin-at-work erin-at-work deleted the fix/window-type branch March 25, 2022 19:50
bangtoven pushed a commit that referenced this pull request Feb 29, 2024
@erin-at-work
Update window.ethereum type to unknown (#432)
5b0fd5e 
* Update window.ethereum type to any

* Updates to 'unknown'

* Add comment

* Use JSDOC comments for deprecated api
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@bangtoven bangtoven bangtoven approved these changes

@jeongmincho jeongmincho Awaiting requested review from jeongmincho

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@erin-at-work @cb-heimdall @ljharb @bangtoven