Skip to content

[apps][part 2] use common utilities from @mysten/core inside the wallet and explorer #8632

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

Merged
merged 20 commits into from
Mar 3, 2023

Conversation

williamrobertson13
Copy link
Contributor

@williamrobertson13 williamrobertson13 commented Feb 24, 2023

Description

This is a follow-up to #8623, so now we're actually using useRpcClient, SentryRpcClient, and useFormatCoin from @mysten/core in our frontend applications. If you're building features right now, you'll need to rebase and make sure you're using useRpcClient from @mysten/core when making RPC calls!

(Also apologies in advance for this size of this changeset, it's mostly updating imports)

Test Plan

  • Manual testing (switching networks works in both the wallet and explorer)
  • CI

If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

  • user-visible impact
  • breaking change for a client SDKs
  • breaking change for FNs (FN binary must upgrade)
  • breaking change for validators or node operators (must upgrade binaries)
  • breaking change for on-chain data layout
  • necessitate either a data wipe or data migration

Release notes

N/A

@vercel
Copy link

vercel bot commented Feb 24, 2023

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

3 Ignored Deployments
Name Status Preview Comments Updated
explorer ⬜️ Ignored (Inspect) Visit Preview Mar 3, 2023 at 4:06PM (UTC)
explorer-storybook ⬜️ Ignored (Inspect) Visit Preview Mar 3, 2023 at 4:06PM (UTC)
wallet-adapter ⬜️ Ignored (Inspect) Visit Preview Mar 3, 2023 at 4:06PM (UTC)

@vercel vercel bot temporarily deployed to Preview – explorer-storybook February 24, 2023 22:26 Inactive
@vercel vercel bot temporarily deployed to Preview – explorer-storybook February 24, 2023 22:30 Inactive
@williamrobertson13 williamrobertson13 force-pushed the wrobertson/use_mysten_core_pt2 branch from e552fc5 to 8a34df9 Compare February 24, 2023 22:31
@vercel vercel bot temporarily deployed to Preview – explorer-storybook February 24, 2023 22:33 Inactive
@williamrobertson13 williamrobertson13 force-pushed the wrobertson/use_mysten_core branch from 5f4f522 to 6c205db Compare February 27, 2023 14:39
@williamrobertson13 williamrobertson13 force-pushed the wrobertson/use_mysten_core_pt2 branch from 8a34df9 to 665af92 Compare February 27, 2023 14:44
@vercel vercel bot temporarily deployed to Preview – explorer-storybook February 27, 2023 14:45 Inactive
@williamrobertson13 williamrobertson13 changed the title [apps][part 2][WIP] use common utilities from @mysten/core inside the wallet and explorer [apps][part 2] use common utilities from @mysten/core inside the wallet and explorer Feb 27, 2023
@williamrobertson13 williamrobertson13 force-pushed the wrobertson/use_mysten_core_pt2 branch from 665af92 to 6d0624b Compare February 27, 2023 14:52
@vercel vercel bot temporarily deployed to Preview – explorer-storybook February 27, 2023 14:53 Inactive
@williamrobertson13 williamrobertson13 self-assigned this Feb 27, 2023
@williamrobertson13 williamrobertson13 marked this pull request as ready for review February 27, 2023 14:57
@williamrobertson13 williamrobertson13 marked this pull request as draft February 27, 2023 15:13
@williamrobertson13 williamrobertson13 marked this pull request as ready for review February 27, 2023 16:25
@vercel vercel bot temporarily deployed to Preview – wallet-adapter February 27, 2023 16:25 Inactive
@vercel vercel bot temporarily deployed to Preview – explorer-storybook February 27, 2023 16:25 Inactive
.npmrc Outdated
@@ -1,2 +1,3 @@
strict-peer-dependencies=false
auto-install-peers=true
Copy link
Contributor Author

@williamrobertson13 williamrobertson13 Feb 27, 2023

Choose a reason for hiding this comment

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

There was a slightly tricky issue where the dependency for react-query in @mysten/core created a separate module installation in pnpm-lock.yaml resulting in two different react-query contexts (one for the wallet/explorer and one for @mysten/core).

The fix for this was to make react, react-dom, and react-query peer dependencies in @mysten/core, and so I flipped auto-install-peers to true so we can actually use those modules inside our core library. I figured this would be fine since regular npm automatically installs peer dependencies by default 🤷🏼‍♂️ , but LMK otherwise if there are any concerns here! cc @Jordan-Mysten

TanStack/query#3595 (comment)
pnpm/pnpm#5351

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love this, I think we either should:

  1. Enforce single versions of packages, which is generally pretty easy to do by making sure versions in package.json match then usingpnpm-deduplicate.
  2. Disable this setting, add all peer deps as dev deps (letting them be used in anything we do in core easily), and ensure peers are correctly setup.

I generally prefer 1 here.

Copy link
Contributor Author

@williamrobertson13 williamrobertson13 Feb 27, 2023

Choose a reason for hiding this comment

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

Ahhhh I figured out why two separate entries were being created in pnpm-lock.yaml, I missed adding react-dom as a dependency to @mysten/core 🤦🏼‍♂️. I reverted the change to .npmrc, so I think we should be good here.

There should be only a single version of react-query now:

  /@tanstack/react-query/4.22.0_biqbaboplfbrettd7655fr4n2y:
    resolution: {integrity: sha512-P9o+HjG42uB/xHR6dMsJaPhtZydSe4v0xdG5G/cEj1oHZAXelMlm67/rYJNQGKgBamKElKogj+HYGF+NY2yHYg==}
    peerDependencies:
      react: ^16.8.0 || ^17.0.0 || ^18.0.0
      react-dom: ^16.8.0 || ^17.0.0 || ^18.0.0
      react-native: '*'
    peerDependenciesMeta:
      react-dom:
        optional: true
      react-native:
        optional: true
    dependencies:
      '@tanstack/query-core': 4.22.0
      react: 18.2.0
      react-dom: [email protected]
      use-sync-external-store: [email protected]
    dev: false

@vercel vercel bot temporarily deployed to Preview – explorer-storybook March 3, 2023 15:39 Inactive
@vercel vercel bot temporarily deployed to Preview – explorer-storybook March 3, 2023 15:40 Inactive
@williamrobertson13 williamrobertson13 merged commit a343ec6 into main Mar 3, 2023
@williamrobertson13 williamrobertson13 deleted the wrobertson/use_mysten_core_pt2 branch March 3, 2023 16:19
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.

2 participants