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

feat: add createExternalExtensionProvider #152

Merged
merged 4 commits into from
May 3, 2021
Merged

feat: add createExternalExtensionProvider #152

merged 4 commits into from
May 3, 2021

Conversation

shanejonas
Copy link
Contributor

  • remove index.d.ts

@shanejonas shanejonas requested a review from a team as a code owner April 30, 2021 21:18
@shanejonas shanejonas force-pushed the add-create-external-extension-provider branch from a872031 to 22f6f35 Compare April 30, 2021 21:19
shanejonas
shanejonas commented Apr 30, 2021
View reviewed changes
src/createMetaMaskExternalExtensionProvider.ts Outdated
const currentMetaMaskId = getMetaMaskId();
const metamaskPort = chrome.runtime.connect(
currentMetaMaskId,
) as Runtime.Port;
Copy link
Contributor Author

@shanejonas shanejonas Apr 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I have to cast this to play nicely with the webextension-polyfill-ts type that extension-port-stream uses, doesnt seem to match exactly to the type chrome.runtime.connect returns which is a chrome.runtime.Port

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Port- and stream-related types seem to be incompatible 100% of the time.

@rekmarks rekmarks self-assigned this May 3, 2021
rekmarks
rekmarks requested changes May 3, 2021
View reviewed changes
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

Starting to look at this now. We should delete index.d.ts in a separate PR.

rekmarks
rekmarks requested changes May 3, 2021
View reviewed changes
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

Looks good! I have one request and one question:

Request: Since there are multiple of them, can we move all of the extension-related files into a new folder src/extension-provider?

Question: I'd like us to only use the BaseProvider instead of the inpage provider, to force consumers of this off the legacy API. What do you think of that?

src/createMetaMaskExternalExtensionProvider.ts Outdated
const currentMetaMaskId = getMetaMaskId();
const metamaskPort = chrome.runtime.connect(
currentMetaMaskId,
) as Runtime.Port;
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Port- and stream-related types seem to be incompatible 100% of the time.

@shanejonas
Copy link
Contributor Author

index.d.ts removal in this PR: #153

@shanejonas shanejonas requested a review from rekmarks May 3, 2021 20:47
@shanejonas
Copy link
Contributor Author

shanejonas commented May 3, 2021 •
edited
Loading

added src/extension-provider/

@shanejonas shanejonas force-pushed the add-create-external-extension-provider branch from 00379bc to db5d701 Compare May 3, 2021 22:40
@shanejonas shanejonas force-pushed the add-create-external-extension-provider branch from db5d701 to 82eaee5 Compare May 3, 2021 22:55
@shanejonas shanejonas changed the title feat: add createMetaMaskExternalExtensionProvider feat: add createExternalExtensionProvider May 3, 2021
rekmarks
rekmarks approved these changes May 3, 2021
View reviewed changes
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM!

@shanejonas shanejonas merged commit 8b12d04 into main May 3, 2021
@shanejonas shanejonas deleted the add-create-external-extension-provider branch May 3, 2021 23:34
@rekmarks rekmarks mentioned this pull request May 5, 2021
Merged
legobeat added a commit to legobeat/providers that referenced this pull request May 10, 2023
@legobeat
move @types/ packages from deps to devDeps
86f8921 
Originally introduced in MetaMask#152
@legobeat legobeat mentioned this pull request May 10, 2023
Merged
legobeat added a commit to legobeat/providers that referenced this pull request May 10, 2023
@legobeat
move @types/ packages from deps to devDeps
821d1cd 
Originally introduced in:
- MetaMask#152
- MetaMask#249
legobeat added a commit that referenced this pull request May 10, 2023
@legobeat
move @types/ packages from deps to devDeps (#259)
41baf7f 
Originally introduced in:
- #152
- #249
@snyk-bot snyk-bot mentioned this pull request Jun 5, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rekmarks rekmarks rekmarks approved these changes

Assignees

@rekmarks rekmarks

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shanejonas @rekmarks