-
-
Notifications
You must be signed in to change notification settings - Fork 187
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: make SafeEventEmitterProvider compatible with eth req libraries #4422
Merged
cryptodev-2s
merged 32 commits into
main
from
feature/make-SafeEventEmitterProvider-type-compatible-with-ethereum-request-libraries
Jul 8, 2024
Merged
Changes from 25 commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
fe4e75b
feat: make SafeEventEmitterProvider compatible with eth req libraries
cryptodev-2s fd56e13
feat: add tsd for type testing
cryptodev-2s d754397
fix: unit tests
cryptodev-2s 72a25cd
Merge branch 'main' into feature/make-SafeEventEmitterProvider-type-c…
cryptodev-2s e734a5d
fix: build
cryptodev-2s bd6070f
fix: test coverage
cryptodev-2s 3bbf4db
Merge branch 'main' into feature/make-SafeEventEmitterProvider-type-c…
cryptodev-2s 2bd3b24
Merge branch 'main' into feature/make-SafeEventEmitterProvider-type-c…
cryptodev-2s 5338acc
fix: override request in fake provider
cryptodev-2s dc2f57b
fix: helper function unit tests
cryptodev-2s c99a02c
fix: remove tsd
cryptodev-2s 2285231
fix: remove useless jest config update
cryptodev-2s 7783d95
Merge branch 'main' into feature/make-SafeEventEmitterProvider-type-c…
cryptodev-2s 403a862
Merge branch 'main' into feature/make-SafeEventEmitterProvider-type-c…
cryptodev-2s 9f40d0e
Merge remote-tracking branch 'origin/main' into feature/make-SafeEven…
cryptodev-2s 58443b4
fix: add smoke tests
cryptodev-2s c96d681
fix: move @metamask/utils to devDependencies
cryptodev-2s e87a334
fix: move back metamsk utils as dependencies
cryptodev-2s 662b20e
fix: dependencies order
cryptodev-2s cf969dc
fix: request returned type and result
cryptodev-2s efda64b
Merge branch 'main' into feature/make-SafeEventEmitterProvider-type-c…
cryptodev-2s 5a7887f
fix: remove useless export of Eip1193Request
cryptodev-2s 640e821
fix: fake-provider
cryptodev-2s 5ac0498
fix: test handle thrown error
cryptodev-2s 17118e6
fix: remove optional chaining operator
cryptodev-2s 8a9dd51
fix: thrown error
cryptodev-2s b3c426a
fix: add more tests for request method
cryptodev-2s 88162ec
fix: add more tests for sendAsync and send
cryptodev-2s 43608ec
fix: add more request failure tests
cryptodev-2s 38dbabd
fix: request error unit tests
cryptodev-2s e19d25a
fix: drop error line
cryptodev-2s e9c69bb
fix: add :
cryptodev-2s File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is only used for dependencies?
Could the types be re-exported in this package, thus delegating
@metamask/utils
to adevDependency
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To complete it, the types used in exports also have to be exported from
packages/eth-json-rpc-provider/src/index.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@legobeat I'm not sure I understand the reasoning for this change, based on the criteria for categorizing dep vs. devDep discussed here: #4449 (comment).
The types imported from
@metamask/utils
always need to be present for the user, since they are dependencies for the exported types and functionsEip1193Request
,convertEip1193RequestToJsonRpcRequest
,SafeEventEmitterProvider['request']
,SafeEventEmitterProvider['sendAsync']
,SafeEventEmitterProvider['send']
. These exports will always directly reference the imported types, and "any project using this package would need to have the package the types come from as well, or it would be an invalid reference."The type imports from
@metamask/utils
are also present in the built type declaration files for this package, which wouldn't happen if they were only being used internally at build time.Re-exporting the imports would give the user access to them, but wouldn't it also require the user to manually import the re-exported types in order to use the exports from this package -- even if the re-exported types are not used anywhere else? It seems like that would effectively make
@metamask/utils
a package dependency, just with extra steps.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this as the key criterion for deciding that
@metamask/utils
should be independencies
and notdevDependencies
, so that TypeScript users are able to use the type declarations that ship with this package.I see that we are exporting
Eip1193Request
in this PR. If we were not doing this, then it would be less obvious that the type declaration files referenced types imported from@metamask/utils
, but I believe that even without this,providerFromMiddleware
takes types that referenceJson
andJsonRpcParams
. So I believe that@metamask/utils
still needs to be included independencies
.Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion here is that the used types (
Json
,JsonRpcParams
) would get re-export
-ed from this package.This will be equivalent from the TypeScript user perspective (the exact same types are available for them) but without requiring the downstream user to pull in the not-actually used js files. So effectively the built
.d.ts
files in@metamask/eth-json-rpc-provider
would include the necessary definitions which would otherwise get pulled in from@metamask/utils
.TypeScript will transparently see that the types are identical and not distinguish between them when imported from different sources downstream.
This also has a side-benefit that any future otherwise non-breaking module refactors (renaming or moving initial type definitions between different packages, for example) becomes transparent for users of this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Is the thought here that if we re-export types from
@metamask/utils
, the TypeScript compiler will recognize this and embed those types into the type declarations for@metamask/eth-json-rpc-provider
? If so, I'm not sure it works that way. I believe the type declaration files would still includeimport
lines from@metamask/utils
, even if those types are being re-exported. Therefore, the consumer would still need@metamask/utils
somewhere in the dependency tree for TypeScript to "see" those types.