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

injected.ts - check shimDisconnect before requestPermissions #3608

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

mqklin
Copy link
Contributor

@mqklin mqklin commented Feb 16, 2024

Following #3603

Description

shimDisconnect should be considered before showing prompt for selecting account

Additional Information

Before submitting this issue, please make sure you do the following.

  • Read the contributing guide
  • Added documentation related to the changes made.
  • Added or updated tests (and snapshots) related to the changes made.

Copy link

changeset-bot bot commented Feb 16, 2024

🦋 Changeset detected

Latest commit: 46e33ce

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@wagmi/core Patch
@wagmi/connectors Patch
wagmi Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Feb 16, 2024

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

Name Status Preview Comments Updated (UTC)
wagmi ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 27, 2024 6:49pm

@mqklin
Copy link
Contributor Author

mqklin commented Feb 16, 2024

From my understanding there is only one-line change? Also not sure what to do with changeset..

@mqklin mqklin marked this pull request as draft February 16, 2024 18:32
@mqklin mqklin marked this pull request as ready for review February 16, 2024 19:29
@mqklin
Copy link
Contributor Author

mqklin commented Feb 16, 2024

I tested it on SanR.app codebase, no bugs encountered.

@mqklin
Copy link
Contributor Author

mqklin commented Feb 21, 2024

@tmm any chance for this to be reviewed / merged / released?

@mqklin
Copy link
Contributor Author

mqklin commented Feb 24, 2024

Co-authored-by: jxom  <jakemoxey@gmail.com>
@mqklin
Copy link
Contributor Author

mqklin commented Mar 4, 2024

So I believe the way to resolve is 1) merge this PR 2) fix the bug I described here: #3608 (comment)

IDK how to fix it. shimDisconnect is only passed to injected, but how to save it and pass further to createConfig? I see 3 ways:

  1. pass shimDisconnect to createConfig, but this duplicates the code:
const wagmiConfig = createConfig({
  ...,
  shimDisconnect: false,
  connectors: [
    injected({shimDisconnect: false}),
  ],
});
  1. Save shimDisconnect as a closure in injected.ts, which looks hacky:
let shimDisconnect;
export function injected(parameters = {}) {
  shimDisconnect ??= parameters.shimDisconnect ?? true;
  const { unstable_shimAsyncInject } = parameters;
  ...

To make it work "clear", shimDisconnect should be passed to midp lib, which is I believe not what you want. So seems like 2nd option I provided is the best way to fix the bug.

  1. nd option is to save shimDisconnect inside createConnector config:
...
        get shimDisconnect() {
          return shimDisconnect;
        }

Then create connectors inside createConfig just to get this shimDisconnect:

...
  const shimDisconnect = rest.connectors.map(f => f()).some(connector => connector.shimDisconnect);
  ...
  ? mipd?.getProviders().map(providerDetailToConnector.bind(null, shimDisconnect)) ?? []
...

And receive it in providerDetailToConnector function below:

...
    function providerDetailToConnector(providerDetail, shimDisconnect) {
        const { info } = providerDetail;
        const provider = providerDetail.provider;
        return injected({ shimDisconnect, target: { ...info, id: info.rdns, provider } });
    }
...

@mqklin
Copy link
Contributor Author

mqklin commented Mar 4, 2024

@jxom please let me know if you want me to PR 2nd option I provided above

.changeset/two-sheep-shop.md Outdated Show resolved Hide resolved
.changeset/two-sheep-shop.md Outdated Show resolved Hide resolved
@tmm tmm merged commit 0acd313 into wevm:main Mar 27, 2024
5 of 10 checks passed
@mqklin mqklin mentioned this pull request Apr 16, 2024
1 task
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.

3 participants