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

Replace superstruct with ESM-compatible fork @metamask/superstruct #2445

Merged
merged 59 commits into from
Jul 17, 2024

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented May 30, 2024

Explanation

As part of the Wallet Framework Team's OKR (Q2 2024 O3KR4, Q3 2024 O2KR4) for upgrading TypeScript to v5.0+ in the core monorepo, we are updating dependencies of the core repo so that they can be used with projects that use Node16 or NodeNext as their moduleResolution tsconfig option.

To achieve this, all dependencies that are ESM packages need to be updated so that they generate separate builds and type declarations that are explicitly designated for CJS and ESM.

This requirement applies to nested dependencies as well, so we are also replacing superstruct with the ESM-compatible fork @metamask/superstruct in all dependencies of core that have superstruct as a dependency.

Description

  • Replace superstruct dependency with @metamask/superstruct ^3.0.0.
    • ^3.1.0
  • Replace all superstruct import statements with @metamask/superstruct
  • Bump @metamask/utils to ^8.5.0.
    • ^9.1.0
      • remove yarn resolution to @metamask/superstruct@npm:3.1.0
  • If feasible without too much additional work: -> create separate PRs for these tasks

Further context on why the superstruct and utils changes are necessary:

yarn resolutions

@metamask/utils is pinned to ^9.1.0 via yarn resolutions, as there are a large number of dependencies that are set to ^8.5.0 (see below), and some of them (especially the core packages) are blocked by the merge and release of this PR:

  • core monorepo: approval-controller, base-controller, controller-utils, eth-json-rpc-provider, json-rpc-engine, json-rpc-middleware-stream, permission-controller
  • standalone: browser-passworder, create-release-branch, eth-block-tracker, eth-json-rpc-middleware, eth-sig-util, key-tree, post-message-stream, providers, snaps-registry

Mixed usage of utils v8 and v9 anywhere in the monorepo's dependency tree causes the following type errors:

Release order roadmap

Due to interdependencies between the packages involved in this PR, we will need to update and release them in a specific order:

References

Changelog

@metamask/snaps-cli

### Changed
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))

### Fixed
- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
  - This fixes the issue of this package being unusable by any TypeScript project that uses `Node16` or `NodeNext` as its `moduleResolution` option.

@metamask/snaps-controllers

### Changed
- Bump `@metamask/base-controller` from `^6.0.0` to `^6.0.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/permission-controller` from `^10.0.0` to `^10.0.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/rpc-errors` from `^6.2.1` to `^6.3.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/snaps-registry` from `^3.1.0` to `^3.2.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))

@metamask/snaps-execution-environments

### Changed
- Bump `@metamask/rpc-errors` from `^6.2.1` to `^6.3.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))

### Fixed
- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
  - This fixes the issue of this package being unusable by any TypeScript project that uses `Node16` or `NodeNext` as its `moduleResolution` option.
- Set `@metamask/providers` from `^17.0.0` to `17.0.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
  - `17.1.0` and `17.1.1` introduce regressions.

@metamask/snaps-jest

### Changed
- Bump `@metamask/base-controller` from `^6.0.0` to `^6.0.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/key-tree` from `^9.1.1` to `^9.1.2` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/permission-controller` from `^10.0.0` to `^10.0.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))

### Fixed
- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
  - This fixes the issue of this package being unusable by any TypeScript project that uses `Node16` or `NodeNext` as its `moduleResolution` option.

@metamask/snaps-rpc-methods

### Changed
- Bump `@metamask/key-tree` from `^9.1.1` to `^9.1.2` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/permission-controller` from `^10.0.0` to `^10.0.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/rpc-errors` from `^6.2.1` to `^6.3.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))

### Fixed
- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
  - This fixes the issue of this package being unusable by any TypeScript project that uses `Node16` or `NodeNext` as its `moduleResolution` option.

@metamask/snaps-sdk

### Changed
- Bump `@metamask/key-tree` from `^9.1.1` to `^9.1.2` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/rpc-errors` from `^6.2.1` to `^6.3.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))

### Fixed
- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
  - This fixes the issue of this package being unusable by any TypeScript project that uses `Node16` or `NodeNext` as its `moduleResolution` option.
- Set `@metamask/providers` from `^17.0.0` to `17.0.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
  - `17.1.0` and `17.1.1` introduce regressions.

@metamask/snaps-simulator (major)

### Changed
- **BREAKING:** Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
  - Due to the return type of `bigIntToHex` being narrowed from `string` to `Hex`, the return type of `hexlifyTransactionData` is narrowed from an object of type `Record<keyof transaction, string>` to an object of type `Record<keyof transaction, Hex>`, where `transaction` is of type `Omit<TransactionFormData, 'transactionOrigin' | 'chainId'>`
- Bump `@metamask/base-controller` from `^6.0.0` to `^6.0.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/key-tree` from `^9.1.1` to `^9.1.2` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/permission-controller` from `^10.0.0` to `^10.0.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))

### Fixed
- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
  - This fixes the issue of this package being unusable by any TypeScript project that uses `Node16` or `NodeNext` as its `moduleResolution` option.

@metamask/snaps-utils

### Changed
- Bump `@metamask/base-controller` from `^6.0.0` to `^6.0.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/key-tree` from `^9.1.1` to `^9.1.2` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/permission-controller` from `^10.0.0` to `^10.0.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/rpc-errors` from `^6.2.1` to `^6.3.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/snaps-registry` from `^3.1.0` to `^3.2.1` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))

### Fixed
- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
  - This fixes the issue of this package being unusable by any TypeScript project that uses `Node16` or `NodeNext` as its `moduleResolution` option.

@metamask/snaps-webpack-plugin

### Changed
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))

@metamask/test-snaps

### Changed
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))

### Fixed
- Set `@metamask/providers` from `^17.0.0` to `17.0.0` ([#2445](https://github.com/MetaMask/snaps/pull/2445))
  - `17.1.0` and `17.1.1` introduce regressions.

@MajorLift MajorLift self-assigned this May 30, 2024
@MajorLift MajorLift force-pushed the replace-superstruct-with-fork branch 2 times, most recently from 5e07dbf to c871235 Compare May 30, 2024 21:30
@MajorLift MajorLift force-pushed the replace-superstruct-with-fork branch 8 times, most recently from 04e666e to 4fc89dd Compare June 7, 2024 01:59
@MajorLift MajorLift force-pushed the replace-superstruct-with-fork branch 3 times, most recently from 291a40f to 0fd099d Compare June 7, 2024 15:07
@MajorLift MajorLift force-pushed the replace-superstruct-with-fork branch from da782f2 to 2df0814 Compare June 14, 2024 15:10
@MajorLift MajorLift force-pushed the replace-superstruct-with-fork branch 3 times, most recently from 3701892 to b9a912b Compare June 14, 2024 18:53
@MajorLift MajorLift force-pushed the replace-superstruct-with-fork branch 4 times, most recently from ddf8aec to aeccdd8 Compare June 24, 2024 15:03
Copy link

codecov bot commented Jun 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.99%. Comparing base (0a265dc) to head (efe17f3).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2445   +/-   ##
=======================================
  Coverage   93.99%   93.99%           
=======================================
  Files         459      459           
  Lines        9647     9647           
  Branches     1480     1480           
=======================================
  Hits         9068     9068           
  Misses        579      579           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MajorLift MajorLift marked this pull request as ready for review June 25, 2024 00:10
@MajorLift MajorLift requested a review from a team as a code owner June 25, 2024 00:10
@MajorLift MajorLift force-pushed the replace-superstruct-with-fork branch from 84f9c86 to 2248718 Compare July 12, 2024 20:09
@MajorLift MajorLift force-pushed the replace-superstruct-with-fork branch 2 times, most recently from 5f94502 to 7dee24c Compare July 15, 2024 16:47
import { StreamProvider } from '@metamask/providers/dist/StreamProvider';
import { StreamProvider } from '@metamask/providers';
Copy link
Member

Choose a reason for hiding this comment

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

For bundle size reasons we need to import StreamProvider from ./dist/StreamProvider directly. The execution environment is built with LavaMoat and it currently doesn't support tree shaking.

Copy link
Contributor Author

@MajorLift MajorLift Jul 15, 2024

Choose a reason for hiding this comment

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

Reverted in latest commit: e92543e

I get the following error from yarn build:lavamoat, which was previously resolved by removing the subpath import.

Error#1: Can't walk dependency graph: Cannot find module '@metamask/providers/dist/StreamProvider' from '/Volumes/Code/metamask/snaps/packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts'

https://github.com/MetaMask/snaps/actions/runs/9946111549/job/27475887863?pr=2445

Copy link
Contributor Author

@MajorLift MajorLift Jul 15, 2024

Choose a reason for hiding this comment

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

Ah appending .cjs seems to work. I'll push that now.

Edit: 69abbc0

Copy link
Contributor Author

@MajorLift MajorLift Jul 15, 2024

Choose a reason for hiding this comment

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

Appending file extensions for subpath imports resolves the lavamoat error, but causes browser test fails in snaps-execution-environment.

[0-5]  Error:  Test failed due to the following error: [plugin:vite:import-analysis] Missing "./dist/StreamProvider.cjs" specifier in "@metamask/providers" package
/home/runner/work/snaps/snaps/packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts

https://github.com/MetaMask/snaps/actions/runs/9946262075/job/27476487392?pr=2445

Copy link
Contributor Author

@MajorLift MajorLift Jul 15, 2024

Choose a reason for hiding this comment

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

Reverted the file extensions, but still getting test:browser errors that don't appear to be directly related to @metamask/providers:

(no CI run link for this because lavamoat prepare step fails)

yarn workspace @metamask/snaps-execution-environments test:browser

Execution of 7 workers started at 2024-07-15T20:52:44.865Z

[0-0] RUNNING in chrome - file:///src/common/BaseSnapExecutor.test.browser.ts
2024-07-15T20:52:47.726Z ERROR @wdio/browser-runner:plugin: No environment found for non determined environment
[0-0]  Error:  Test failed due to following error(s):
  - BaseSnapExecutor.test.browser.ts: Failed to fetch dynamically imported module: http://localhost:52887/src/common/test-utils/endowments.ts: TypeError: Failed to fetch dynamically imported module: http://localhost:52887/src/common/test-utils/endowments.ts

[0-0] FAILED in chrome - file:///src/common/BaseSnapExecutor.test.browser.ts
[0-1] RUNNING in chrome - file:///src/common/endowments/endowments.test.browser.ts
2024-07-15T20:52:56.046Z ERROR @wdio/browser-runner:plugin: No environment found for non determined environment
[0-1]  Error:  Test failed due to following error(s):
  - endowments.test.browser.ts: Failed to fetch dynamically imported module: http://localhost:52917/src/common/test-utils/endowments.ts: TypeError: Failed to fetch dynamically imported module: http://localhost:52917/src/common/test-utils/endowments.ts

[0-1] FAILED in chrome - file:///src/common/endowments/endowments.test.browser.ts
[0-2] RUNNING in chrome - file:///src/common/lockdown/lockdown-events.test.browser.ts
2024-07-15T20:52:58.874Z ERROR @wdio/browser-runner:plugin: No environment found for non determined environment
 Hook executed: "before all" hook: wrappedHook in "{root}"
 ------------------------------------------------------------------
 Suite started: 
   » file:///src/common/lockdown/lockdown-events.test.browser.ts
 lockdown events security
    ✓ should lockdown events and made event properties inaccessible » [ file:///src/common/lockdown/lockdown-events.test.browser.ts ]
 Hook executed: "after all" hook: wrappedHook in "{root}"
[0-2] PASSED in chrome - file:///src/common/lockdown/lockdown-events.test.browser.ts
[0-3] RUNNING in chrome - file:///src/iframe/IFrameSnapExecutor.test.browser.ts
2024-07-15T20:53:01.110Z ERROR @wdio/browser-runner:plugin: No environment found for non determined environment
[0-3] Removing intrinsics.Number.isNaN.getPolyfill
[0-3] Removing intrinsics.Number.isNaN.implementation
[0-3] Removing intrinsics.Number.isNaN.shim
[0-3] MetaMask: Connected to chain with ID "0x1".
 Hook executed: "before all" hook: wrappedHook in "{root}"
 ------------------------------------------------------------------
 Suite started: 
   » file:///src/iframe/IFrameSnapExecutor.test.browser.ts
 IFrameSnapExecutor
 Hook executed: "before all" hook for IFrameSnapExecutor
    ✓ receives and processes commands » [ file:///src/iframe/IFrameSnapExecutor.test.browser.ts ]
 Hook executed: "after all" hook: wrappedHook in "{root}"
[0-3] PASSED in chrome - file:///src/iframe/IFrameSnapExecutor.test.browser.ts
[0-4] RUNNING in chrome - file:///src/proxy/ProxySnapExecutor.test.browser.ts
2024-07-15T20:53:04.866Z ERROR @wdio/browser-runner:plugin: No environment found for non determined environment
 Hook executed: "before all" hook: wrappedHook in "{root}"
 ------------------------------------------------------------------
 Suite started: 
   » file:///src/proxy/ProxySnapExecutor.test.browser.ts
 ProxySnapExecutor
    ✓ forwards messages to the iframe » [ file:///src/proxy/ProxySnapExecutor.test.browser.ts ]
    ✓ terminates the iframe » [ file:///src/proxy/ProxySnapExecutor.test.browser.ts ]
 Hook executed: "after all" hook: wrappedHook in "{root}"
[0-4] PASSED in chrome - file:///src/proxy/ProxySnapExecutor.test.browser.ts
[0-5] RUNNING in chrome - file:///src/webworker/executor/WebWorkerSnapExecutor.test.browser.ts
2024-07-15T20:53:07.651Z ERROR @wdio/browser-runner:plugin: No environment found for non determined environment
[0-5] Removing intrinsics.Number.isNaN.getPolyfill
[0-5] Removing intrinsics.Number.isNaN.implementation
[0-5] Removing intrinsics.Number.isNaN.shim
[0-5] MetaMask: Connected to chain with ID "0x1".
 Hook executed: "before all" hook: wrappedHook in "{root}"
 ------------------------------------------------------------------
 Suite started: 
   » file:///src/webworker/executor/WebWorkerSnapExecutor.test.browser.ts
 WebWorkerSnapExecutor
 Hook executed: "before all" hook for WebWorkerSnapExecutor
 Hook executed: "before each" hook for WebWorkerSnapExecutor
    ✓ receives and processes commands » [ file:///src/webworker/executor/WebWorkerSnapExecutor.test.browser.ts ]
    - handles closing the stream » [ file:///src/webworker/executor/WebWorkerSnapExecutor.test.browser.ts ]
    - handles stream errors » [ file:///src/webworker/executor/WebWorkerSnapExecutor.test.browser.ts ]
 Hook executed: "after all" hook: wrappedHook in "{root}"
[0-5] PASSED in chrome - file:///src/webworker/executor/WebWorkerSnapExecutor.test.browser.ts
[0-6] RUNNING in chrome - file:///src/webworker/pool/WebWorkerPool.test.browser.ts
2024-07-15T20:53:10.854Z ERROR @wdio/browser-runner:plugin: No environment found for non determined environment
[0-6] [Worker] Error initializing job: Error: Failed to fetch.
 Hook executed: "before all" hook: wrappedHook in "{root}"
 ------------------------------------------------------------------
 Suite started: 
   » file:///src/webworker/pool/WebWorkerPool.test.browser.ts
 WebWorkerPool
    ✓ forwards messages to the worker » [ file:///src/webworker/pool/WebWorkerPool.test.browser.ts ]
    ✓ terminates the worker » [ file:///src/webworker/pool/WebWorkerPool.test.browser.ts ]
    ✓ creates a worker pool » [ file:///src/webworker/pool/WebWorkerPool.test.browser.ts ]
    ✓ handles errors » [ file:///src/webworker/pool/WebWorkerPool.test.browser.ts ]
 Hook executed: "after all" hook: wrappedHook in "{root}"
[0-6] PASSED in chrome - file:///src/webworker/pool/WebWorkerPool.test.browser.ts

 "spec" Reporter:
------------------------------------------------------------------
Running: chrome-headless-shell (v126.0.6478.127) on mac
Session ID: 193c5b071316ff7ab9bf1b33a4dbe201

» file:///src/common/lockdown/lockdown-events.test.browser.ts
lockdown events security
   ✓ should lockdown events and made event properties inaccessible

1 passing (444ms)
------------------------------------------------------------------
Running: chrome-headless-shell (v126.0.6478.127) on mac
Session ID: ec22bb9107726745900b327cb7a14d37

» file:///src/iframe/IFrameSnapExecutor.test.browser.ts
IFrameSnapExecutor
   ✓ receives and processes commands

1 passing (1.8s)
------------------------------------------------------------------
Running: chrome-headless-shell (v126.0.6478.127) on mac
Session ID: e0c43d2eff8a5c3576fd4f7d3adcd274

» file:///src/proxy/ProxySnapExecutor.test.browser.ts
ProxySnapExecutor
   ✓ forwards messages to the iframe
   ✓ terminates the iframe

2 passing (953ms)
------------------------------------------------------------------
Running: chrome-headless-shell (v126.0.6478.127) on mac
Session ID: 63c2feb62ae19c285652f1ef4a267c65

» file:///src/webworker/executor/WebWorkerSnapExecutor.test.browser.ts
WebWorkerSnapExecutor
   ✓ receives and processes commands
   - handles closing the stream
   - handles stream errors

1 passing (1s)
2 skipped
------------------------------------------------------------------
Running: chrome-headless-shell (v126.0.6478.127) on mac
Session ID: 2bf2771f5c6d1f849b721d754095f564

» file:///src/webworker/pool/WebWorkerPool.test.browser.ts
WebWorkerPool
   ✓ forwards messages to the worker
   ✓ terminates the worker
   ✓ creates a worker pool
   ✓ handles errors

4 passing (854ms)


Spec Files:      5 passed, 2 failed, 7 total (100% completed) in 00:00:26 undefined 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mrtenz How would you feel about extracting this issue to its own ticket and merging this PR with @metamask/provider set to 17.0.0? It doesn't look like this will cause issues in core.

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 to unblock this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I'll create a new ticket for the 17.1.1 version bump.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created: #2579. Included information in this thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RE top comment: The dist/ subpath imports from @metamask/providers are restored here: e92543e

@MajorLift MajorLift force-pushed the replace-superstruct-with-fork branch from 151b872 to e9b91cd Compare July 15, 2024 20:20
@MajorLift MajorLift force-pushed the replace-superstruct-with-fork branch from 5bc42a3 to 69abbc0 Compare July 15, 2024 20:29
@MajorLift MajorLift force-pushed the replace-superstruct-with-fork branch from 69abbc0 to 68cae24 Compare July 15, 2024 20:33
MajorLift added a commit to MetaMask/core that referenced this pull request Jul 16, 2024
## Explanation

Contains patch releases of the following commit for `base-controller`,
`permission-controller`, and all of their upstream deps/peerDeps:

- #4516

In the packages downstream of the release candidates, I'm using the
`"intentionally-skip"` option, and only bumping the release candidates
to their post-release versions in package.json. This way, our yarn
constraints are satisfied in the main branch.

The version bumps will stay as unreleased changes which we can deploy in
a subsequent release PR. Since the package versions are unchanged, the
updates to the dependencies should be treated the same as any unreleased
version bump, not disrupting the `create-release-branch` script or
`publish-release` action in this PR or the upcoming release PR.

To prevent any issues with inconsistency, I plan to release all of these
downstream packages simultaneously in the immediately subsequent release
(173.0.0).

### Release order roadmap

- [core release 172.0.0] `{base,permission}-controller` 
- [snaps release 56.0.0] `snaps-{sdk,utils,rpc-methods}`
- [core release 173.0.0] all core controllers (except
`snaps-controllers` consumers)
- [snaps release 57.0.0] `snaps-controllers`
- [core release 174.0.0] `{accounts,chain,profile-sync}-controller`

## References

- See #3645
- See MetaMask/snaps#2445

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate

---------

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
MajorLift added a commit to MetaMask/core that referenced this pull request Jul 16, 2024
Includes releases for `@metamask/eth-json-rpc-provider`,
`@metamask/json-rpc-middleware-stream`.

This release allows `@metamask/utils` to be bumped to `9.0.0` in all
dependencies of
`@metamask/{providers,eth-block-tracker,eth-json-rpc-middleware}`

### Release order roadmap

- [[core release 172.0.0](#4517)]
`{base,permission}-controller`
- [[core release 173.0.0](#4519)]
`eth-json-rpc-provider`, `json-rpc-middleware-stream`
- [snaps release 56.0.0] `snaps-{sdk,utils,rpc-methods}`
- [core release 173.0.0] all core controllers (except
`snaps-controllers` consumers)
- [snaps release 57.0.0] `snaps-controllers`
- [core release 174.0.0] `{accounts,chain,profile-sync}-controller`

## References

- See #3645
- See MetaMask/snaps#2445

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
@MajorLift MajorLift requested a review from Mrtenz July 16, 2024 19:30
@MajorLift MajorLift merged commit f84abee into main Jul 17, 2024
156 checks passed
@MajorLift MajorLift deleted the replace-superstruct-with-fork branch July 17, 2024 18:00
MajorLift added a commit to MetaMask/core that referenced this pull request Jul 22, 2024
…ode16` (#3645)

## Explanation

### TypeScript v5.0

As part of the Wallet Framework Team's OKRs (Q2 2024 O3KR4, Q3 2024 O2KR4),
we are upgrading TypeScript to v5.0+ for all packages in the core
monorepo.

These upgrades will give us access to new features, aid us in writing
more type-safe and modern code, and also allow us to reach parity with
Extension and other MetaMask projects.

### `Node16`

In order to maximize the benefits of this upgrade, we are also enabling
the `Node16` setting for the `module` and `moduleResolution` tsconfig
options.

## Motivation

### Interop: CJS modules referencing ESM modules

The core monorepo is a collection of CJS packages, which use CJS module
resolution rules internally, and are treated as CJS modules by Node.js.
This is true despite that fact that these packages are authored in
TypeScript using ESM syntax (`import`/`export` statements) and are set
up to output dual builds for both CJS and ESM.

With `Node16` or `NodeNext` enabled, CJS modules are unable to reference
ESM modules via static/synchronous `import` statements, as TypeScript
assumes them to be compiled down to CJS-only `require` statements.

There are three solutions for this issue, of which we are utilizing the
first two:
1. Update the ESM-only dependency so that it outputs a CJS build and
type declaration as well.
2. Replace the static `import` statements with dynamic import syntax
(which, based on CJS emit rules, are not transformed to `require`
statements).
3. Migrate our module to ESM (by setting `"type": "module"` in
package.json and renaming all of our scripts to *.cjs)

> See
https://www.typescriptlang.org/docs/handbook/modules/reference.html#interoperability-rules

With dependencies that we control or use extensively, we pursue the
first option, as we're doing with `superstruct` and `@metamask/utils`
(and all of the many core dependencies that are downstream of either or
both).

With dependencies that see more limited usage, we are opting for either
the second option (e.g. `multiformats`) or, if available, using a
CJS-compatible alternative (e.g. replacing `lodash-es` with `lodash`).

The third solution of migrating to ESM is the most fundamental,
long-term measure, but we are refraining from it at this stage until we
can make a concerted effort to migrate our codebase as a whole. This is
because any individual ESM migration can cause a cascading effect
through the dependency tree where other packages are now required to
migrate as well.

### Reasons for moving away from `node`/`node10`

The following outlines the motivation for switching to `Node16`, backed
by relevant entries from the official TypeScript documentation.
- Starting with TypeScript v5.0, the previously used `moduleResolution`
setting `node` is renamed to `node10`, and strongly discouraged from
usage.
- "It [reflects the CommonJS module resolution algorithm as it existed
in Node.js versions earlier than
v12](https://www.typescriptlang.org/docs/handbook/modules/reference.html#node10-formerly-known-as-node).
**It should no longer be used.**"
- "Because `node16` and `nodenext` are the only module options that
reflect the complexities of Node.js’s dual module system, they are the
[**only correct module options** for all apps and libraries that are
intended to run in Node.js v12 or
later](https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext),
whether they use ES modules or not."
- The `node10` setting is unable to guarantee correct module resolution
for ESM-only dependencies.
- "[Because Node.js v12 introduced different module resolution rules for
ES
modules,](https://www.typescriptlang.org/docs/handbook/modules/theory.html#module-resolution)
though, it’s a very bad model of modern versions of Node.js. It should
not be used for new projects."
- "`node16` and `nodenext` describe the full range of behavior for
Node.js’s dual-format module system, and **emit files in either CommonJS
or ESM format**. This is different from every other `module` option,
which are runtime-agnostic and force all output files into a single
format, [leaving it to the user to ensure the output is valid for their
runtime.](https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext)"
- The `node10` setting does not support the package.json `"exports"`
field, which is used in our libraries to expose dual builds and type
declarations.
-
https://www.typescriptlang.org/docs/handbook/modules/reference.html#packagejson-exports
- The `Node16` and `NodeNext` settings maximize downstream
compatibility.
- "When compiling a library, you don’t know where the output code will
run, but you’d like it to run in as many places as possible. Using
"module": "nodenext" (along with the implied "moduleResolution":
"nodenext") is the best bet for maximizing the compatibility of the
output JavaScript’s module specifiers, since [it will force you to
comply with Node.js’s stricter rules for import module
resolution.](https://www.typescriptlang.org/docs/handbook/modules/theory.html#module-resolution-for-libraries)"
- "`"moduleResolution": "nodenext"` is only checking that the output
works in Node.js, but in most cases, [module code that works in Node.js
will work in other runtimes and in
bundlers](https://www.typescriptlang.org/docs/handbook/modules/theory.html#module-resolution-for-libraries)"
  
#### `Node16` vs. `NodeNext`

The two settings are currently identical, and `Node16` is intended to
work with all current node versions v16 or higher. If additional
capabilities are added to `NodeNext` or `Node18`/`Node20` that we want
to apply to our codebases, we will be able to introduce them after
checking for disruptive regressions or breaking changes.

#### Relationship with other tsconfig options

- `--module` `nodenext` or `node16` implies and enforces the
`moduleResolution` with the same name (and vice versa).
- `--module` `node16` implies (up to) `--target` `es2022`.
  - `--module` `nodenext` implies (up to) `--target` `esnext`.
>
https://www.typescriptlang.org/docs/handbook/modules/reference.html#implied-and-enforced-options

## Description

- [x] Replace `superstruct` dependency with `@metamask/superstruct`
`^3.0.0`.
  - [x] `^3.1.0`
- [x] Replace all `superstruct` import statements with
`@metamask/superstruct`
- [x] Bump `@metamask/utils` to `^8.5.0`.
  - [x] `^9.0.0`
    - [x] remove yarn resolution to `@metamask/superstruct@npm:3.1.0`
- [x] Bump `typescript` to `~5.0.4`
  - [x] Set `module` and `moduleResolution` tsconfig options to `Node16`
  - [ ] ~#4507
  
Further context on why the `superstruct` and `utils` changes are
necessary:
- MetaMask/utils#144
- MetaMask/superstruct#1
- MetaMask/superstruct#18
- MetaMask/utils#182
- MetaMask/metamask-module-template#247
-
https://www.typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html#considerations-for-bundling-libraries

## Release order roadmap

Due to interdependencies between the packages involved in this PR, we
will need to update and release them in a specific order:

- [x] #4516
- [x] Release `{base,permission}-controller`
- [x] (wait for releases: `snaps-sdk`, `snaps-utils`,
`snaps-controllers`, `keyring-api`)
  - [x] MetaMask/keyring-api#356
  - [x] MetaMask/snaps#2445
  - [x] MetaMask/snaps#2584
  - [x] MetaMask/snaps#2589
- [x] Remove yarn resolutions for `snaps-sdk`, `snaps-utils`,
~`keyring-api`~
- [ ] Merge this PR: #3645
- [x] Set yarn resolution for `@metamask/providers` via
`@metamask/snaps-sdk` to `17.1.1`
- [x] Leave messages in Changelog for affected packages
(`{accounts,chain,profile-sync}-controller`) to hold off on new releases
    ```md
    ### Uncategorized

- Please hold off on new releases of this package until the yarn
resolution for `@metamask/providers` is removed.
- This is blocked by a `@metamask/snaps-sdk` release with
`@metamask/providers` bumped to `>=17.1.1`.
- See: [Fix regressions introduced by
@metamask/providers@17.1.1](MetaMask/snaps#2579)
- Build error fixed by yarn resolution:
[MetaMask/core/actions/runs/10011688901/job/27675682526?pr=3645](https://github.com/MetaMask/core/actions/runs/10011688901/job/27675682526?pr=3645)
    ```
- [ ] Release all core pkgs (especially deps of `snaps-controllers` and
consumers of `utils`)
- Exclude core pkgs that have `snaps-controllers` as dependency, and are
affected by `@metamask/providers` yarn resolution
    - `{accounts,chain,profile-sync}-controller`
- [ ] Release `snaps-controllers`
- [ ] Release `{accounts,chain,profile-sync}-controller`,
`eth-snap-keyring`
(MetaMask/eth-snap-keyring#311)
- [ ] (release remaining `snaps` packages while releasing
`@metamask/utils@9.0.0` version bumps for all dependencies and nested
dependencies)

## References

- Contributes to #3651
- Blocked by:
  - `superstruct`
    - [x] MetaMask/superstruct#18
    - [x] MetaMask/superstruct#24
    - [x] MetaMask/superstruct#25
    - [x] MetaMask/superstruct#28
  - `utils`
    - [x] MetaMask/utils#185
    - [x] MetaMask/utils#191
    - [x] MetaMask/utils#194
    - [x] MetaMask/utils#196
- Blocked by downstream consumers of `superstruct`, `utils`:
  - `abi-utils`: 
    - [x] MetaMask/abi-utils#73
    - [x] MetaMask/abi-utils#78
    - [x] MetaMask/abi-utils#80
    - [x] MetaMask/abi-utils#81
  - `chain-api`: 
    - [x] MetaMask/accounts-chain-api#5
- [x] https://github.com/MetaMask/accounts-chain-api/releases/tag/v0.1.0
  - `eth-simple-keyring`: 
    - [x] MetaMask/eth-simple-keyring#177
    - [x] MetaMask/eth-simple-keyring#178
  - `providers`:
    - [x] MetaMask/providers#336
    - [x] MetaMask/providers#337
- Blocked by MetaMask/providers#340,
ts-bridge/ts-bridge#22
- Causes CI failure:
https://github.com/MetaMask/snaps/actions/runs/9783767567/job/27013136688?pr=2445
    - [x] MetaMask/providers#345
    - [x] MetaMask/providers#347
  - `rpc-errors`: 
    - [x] MetaMask/rpc-errors#147
    - [x] MetaMask/rpc-errors#148    
  - `snaps-registry`:
    - [x] MetaMask/snaps-registry#613
    - [x] MetaMask/snaps-registry#670
    - [x] MetaMask/snaps-registry#693
    - [x] MetaMask/snaps-registry#694
- Blocked by `snaps` monorepo releases
  - `snaps-sdk`, `snaps-utils`
    - `keyring-api`
      - [x] MetaMask/keyring-api#328
  - `snaps-controllers` 
    - `eth-snap-keyring`
      - [x] MetaMask/eth-snap-keyring#311

## Changelog

### `@metamask/accounts-controller`

```md
### Changed

- Bump `@metamask/keyring-api` from `^8.0.0` to `^8.0.1` ([#3645](#3645))
- Bump `@metamask/snaps-sdk` from `^4.2.0` to `^6.1.0` ([#3645](#3645))
- Bump `@metamask/snaps-utils` from `^7.4.0` to `^7.8.0` ([#3645](#3645))
- Bump peer dependency `@metamask/snaps-controllers` from `^8.1.1` to `^9.3.0` ([#3645](#3645))
```

### `@metamask/assets-controllers` (major)

```md
### Changed

- **BREAKING:** `getIpfsCIDv1AndPath`, `getFormattedIpfsUrl` are now async functions ([#3645](#3645))
- Add `immer` `^9.0.6` as a new dependenc. ([#3645](#3645))
- Bump `@metamask/abi-utils` from `^2.0.2` to `^2.0.3` ([#3645](#3645))
- Bump `multiformats` from `^9.5.2` to `^13.1.0` ([#3645](#3645))
```

### `@metamask/chain-controller`

```md
### Changed

- Bump `@metamask/chain-api` from `^0.0.1` to `^0.1.0` ([#3645](#3645))
- Bump `@metamask/keyring-api` from `^8.0.0` to `^8.0.1` ([#3645](#3645))
- Bump `@metamask/snaps-controllers` from `^8.1.1` to `^9.3.0` ([#3645](#3645))
- Bump `@metamask/snaps-sdk` from `^4.2.0` to `^6.1.0` ([#3645](#3645))
- Bump `@metamask/snaps-utils` from `^7.4.0` to `^7.8.0` ([#3645](#3645))
```

### `@metamask/keyring-controller`

```md
### Changed

- Bump `@metamask/eth-simple-keyring` from `^6.0.1` to `^6.0.2` ([#3645](#3645))
- Bump `@metamask/keyring-api` from `^8.0.0` to `^8.0.1` ([#3645](#3645))
- Set yarn resolution for `@metamask/snaps-sdk/@metamask/providers` to `17.1.1` ([#3645](#3645))
  - Remove once `@metamask/snaps-sdk` bumps its `@metamask/providers` version to `>=17.1.1`.
```
  
### `@metamask/network-controller` (minor)

```md
### Added

- Newly exports the following types: `AutoManagedNetworkClient`, `InfuraNetworkClientConfiguration`, `CustomNetworkClientConfiguration` ([#3645](#3645))
```

### `@metamask/profile-sync-controller`

```md
### Changed

- Bump dependency and peer dependency `@metamask/snaps-controllers` from `^8.1.1` to `^9.3.0` ([#3645](#3645))
- Bump `@metamask/snaps-sdk` from `^4.2.0` to `^6.1.0` ([#3645](#3645))
- Bump `@metamask/snaps-utils` from `^7.4.0` to `^7.8.0` ([#3645](#3645))
```

### `@metamask/transaction-controller`

```md
### Changed

- Bump `@metamask/keyring-api` from `^8.0.0` to `^8.0.1` ([#3645](#3645))
```

### `@metamask/user-operation-controller`

```md
### Fixed

- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#3645](#3645))
  - This fixes the issue of this package being unusable by any TypeScript project that uses `Node16` or `NodeNext` as its `moduleResolution` option.
```

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate

---------

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace superstruct with ESM-compatible fork @metamask/superstruct
2 participants