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

Upgrade to TypeScript v5.0 and set module{,Resolution} option to Node16 #3645

Merged
merged 61 commits into from
Jul 22, 2024

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Dec 8, 2023

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.

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

  • 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.0.0
      • remove yarn resolution to @metamask/superstruct@npm:3.1.0
  • Bump typescript to ~5.0.4

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

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/accounts-controller

### Changed

- Bump `@metamask/keyring-api` from `^8.0.0` to `^8.0.1` ([#3645](https://github.com/MetaMask/core/pull/3645))
- Bump `@metamask/snaps-sdk` from `^4.2.0` to `^6.1.0` ([#3645](https://github.com/MetaMask/core/pull/3645))
- Bump `@metamask/snaps-utils` from `^7.4.0` to `^7.8.0` ([#3645](https://github.com/MetaMask/core/pull/3645))
- Bump peer dependency `@metamask/snaps-controllers` from `^8.1.1` to `^9.3.0` ([#3645](https://github.com/MetaMask/core/pull/3645))

@metamask/assets-controllers (major)

### Changed

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

@metamask/chain-controller

### Changed

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

@metamask/keyring-controller

### Changed

- Bump `@metamask/eth-simple-keyring` from `^6.0.1` to `^6.0.2` ([#3645](https://github.com/MetaMask/core/pull/3645))
- Bump `@metamask/keyring-api` from `^8.0.0` to `^8.0.1` ([#3645](https://github.com/MetaMask/core/pull/3645))
- Set yarn resolution for `@metamask/snaps-sdk/@metamask/providers` to `17.1.1` ([#3645](https://github.com/MetaMask/core/pull/3645))
  - Remove once `@metamask/snaps-sdk` bumps its `@metamask/providers` version to `>=17.1.1`.

@metamask/network-controller (minor)

### Added

- Newly exports the following types: `AutoManagedNetworkClient`, `InfuraNetworkClientConfiguration`, `CustomNetworkClientConfiguration` ([#3645](https://github.com/MetaMask/core/pull/3645))

@metamask/profile-sync-controller

### Changed

- Bump dependency and peer dependency `@metamask/snaps-controllers` from `^8.1.1` to `^9.3.0` ([#3645](https://github.com/MetaMask/core/pull/3645))
- Bump `@metamask/snaps-sdk` from `^4.2.0` to `^6.1.0` ([#3645](https://github.com/MetaMask/core/pull/3645))
- Bump `@metamask/snaps-utils` from `^7.4.0` to `^7.8.0` ([#3645](https://github.com/MetaMask/core/pull/3645))

@metamask/transaction-controller

### Changed

- Bump `@metamask/keyring-api` from `^8.0.0` to `^8.0.1` ([#3645](https://github.com/MetaMask/core/pull/3645))

@metamask/user-operation-controller

### Fixed

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

Checklist

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

@mcmire
Copy link
Contributor

mcmire commented Mar 14, 2024

I worked on this a bit and by bumping some packages and also using a local copy of utils which makes use of @metamask/superstruct instead of superstruct, I managed to get yarn build:types working. I created a separate branch and made a commit there with the necessary changes, and you can see the package bumps here: df14ce8. You'll want to use your own local copy of utils with the use-metamask-superstruct branch checked out (make sure to run yarn build first).

I haven't tried running yarn test yet, so I don't know if that works. I will try that next.

@MajorLift MajorLift force-pushed the 231208-typescript-module-resolution branch from ee34373 to 5b2d07d Compare May 9, 2024 15:30
@MajorLift MajorLift force-pushed the 231208-typescript-module-resolution branch from 3ee4229 to 26937bc Compare May 22, 2024 00:57
@MajorLift MajorLift self-assigned this May 22, 2024
@MajorLift MajorLift changed the title Update tsconfig module, moduleResolution options to Node16 Update tsconfig module, moduleResolution options to NodeNext May 22, 2024
@MajorLift MajorLift force-pushed the 231208-typescript-module-resolution branch 3 times, most recently from 49aa7ea to 92db09e Compare May 22, 2024 21:32
@MajorLift MajorLift force-pushed the 231208-typescript-module-resolution branch from 92db09e to 94b220f Compare May 31, 2024 20:31
@MajorLift MajorLift changed the title Update tsconfig module, moduleResolution options to NodeNext Bump TypeScript to ^5.0.4 and set module, moduleResolution to NodeNext May 31, 2024
@MajorLift MajorLift force-pushed the 231208-typescript-module-resolution branch 2 times, most recently from f5d774c to e0487db Compare June 3, 2024 01:27
@MetaMask MetaMask deleted a comment from socket-security bot Jun 3, 2024
@MajorLift MajorLift force-pushed the 231208-typescript-module-resolution branch 3 times, most recently from b459ff2 to 5cc2651 Compare June 3, 2024 20:06
This was referenced Jun 5, 2024
Copy link

socket-security bot commented Jun 25, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: npm/micromatch@4.0.7, npm/node-gyp@10.2.0, npm/validate-npm-package-name@5.0.1

View full report↗︎

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

@MajorLift MajorLift force-pushed the 231208-typescript-module-resolution branch from 0454e0e to 903468e Compare June 25, 2024 19:45
@MajorLift MajorLift force-pushed the 231208-typescript-module-resolution branch from 3887186 to 09b29ca Compare July 17, 2024 17:46
MajorLift added a commit to MetaMask/snaps that referenced this pull request Jul 17, 2024
#2445)

## Explanation

As part of the Wallet Framework Team's OKR ([Q2 2024
O3KR4](https://docs.google.com/document/d/1NYWepE--9HLG9NHAxmsHKQI7lfZL2nW0kv7UVd4q160/edit#bookmark=kix.h65bod9heydk),
[Q3 2024
O2KR4](https://docs.google.com/document/d/1JLEzfUxHlT8lw8ntgMWG0vQb5BAATcrYZDj0wRB2ogI/edit#bookmark=kix.yxz96lxhvjk3))
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

- [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.1.0`
    - [x] remove yarn resolution to `@metamask/superstruct@npm:3.1.0`
- [ ] ~If feasible without too much additional work:~ -> create separate
PRs for these tasks
  - [ ] ~Bump `typescript` to `~5.0.4`~
  - [ ] ~#2514

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

### `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:
-
https://github.com/MetaMask/snaps/actions/runs/9720788583/job/26900545288?pr=2445

### 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:

- [ ] Merge this PR: #2445
- Request snaps team to hold off on releases until yarn resolution for
utils can be removed
- Bump `@metamask/utils` in dependencies of
`snaps-{sdk,utils,rpc-methods}`:
    - [x] `rpc-errors`: 
      - [x] MetaMask/rpc-errors#147
      - [x] MetaMask/rpc-errors#148
    - [x] `key-tree`
      - [x] MetaMask/key-tree#181
      - [x] MetaMask/key-tree#182
    - [x] `snaps-registry`
      - [x] MetaMask/snaps-registry#693
      - [x] MetaMask/snaps-registry#694
    - [x] `base-controller`, `permission-controller`
      - [x] MetaMask/core#4516
      - [x] MetaMask/core#4517
- [ ] Release `snaps-sdk`
- [x] Release `keyring-api`:
MetaMask/keyring-api#328
    - [ ] Release `snaps-utils`
      - [ ] Release `snaps-rpc-methods`
- [ ] Merge core PR: MetaMask/core#3645
- Before merging, first remove yarn resolutions for `snaps-sdk`,
`snaps-utils`, `keyring-api`
- [ ] Release all core pkgs (especially deps of `snaps-controllers` and
consumers of `utils`)
- Exclude core pkgs that have `snaps-controllers` as dependency:
`{accounts,chain,profile-sync}-controller`
- [ ] Bump and release all remaining `@metamask/utils@8.x.x` usage in
the `snaps-controllers` dependency tree
  - [x] `browser-passworder`
    - [x] MetaMask/browser-passworder#63
    - [x] MetaMask/browser-passworder#64
  - [x] `post-message-stream`
    - [x] MetaMask/post-message-stream#140
- ~Blocked by build error
https://github.com/MetaMask/post-message-stream/actions/runs/9863225191/job/27235524010?pr=140~
    - [x] MetaMask/post-message-stream#141
- [ ] Release `snaps-controllers`
- [ ] Release `{accounts,chain,profile-sync}-controller`,
`eth-snap-keyring`
(MetaMask/eth-snap-keyring#311)
- [ ] Bump and release all remaining `@metamask/utils@8.x.x` usage in
the snaps monorepo dependency tree
  - [ ] `create-release-branch`
    - [x] MetaMask/create-release-branch#150
    - [x] MetaMask/create-release-branch#149
  - [ ] `eth-block-tracker`
    - ~Blocked by `eth-json-rpc-provider`~
- [ ] Blocked by MetaMask/eth-block-tracker#252
  - [ ] `eth-json-rpc-middleware`
- Blocked by `eth-block-tracker`, ~`eth-json-rpc-provider`,~
`eth-sig-util`, ~`json-rpc-engine`~
  - [x] `abi-utils`
    - [x] MetaMask/abi-utils#80
    - [x] MetaMask/abi-utils#81
  - [ ] `eth-sig-util`
    - ~Blocked by `abi-utils`~
    - [x] MetaMask/eth-sig-util#381
    - [x] MetaMask/eth-sig-util#382
  - [x] `providers`
- ~Blocked by `json-rpc-engine`, `json-rpc-middleware-stream`,
`rpc-errors`~
    - [x] MetaMask/providers#345
    - [x] MetaMask/providers#347
- [ ] Remove yarn resolution for `@metamask/utils`
- [ ] Release all remaining snaps pkgs
  - New snaps monorepo releases are now safe to publish.

## References

- Closes #2444
- Followed by #2514
- Blocked by type errors caused by simultaneous usage of
`@metamask/utils` `9.0.0` and `8.5.0` in dependency tree.
- All tests passing with `@metamask/utils` fixed to `9.0.0` in yarn
resolutions:

https://github.com/MetaMask/snaps/actions/runs/9745954683/job/26895208572?pr=2445
- [ ] `snaps-sdk`, `snaps-utils` and their dependencies that use
`@metamask/utils@8.5.0` are blocking:
- [ ] core release via MetaMask/core#3645, which
is blocking:
      - [ ] `snaps-controllers` type errors
    - [ ] `snaps-rpc-methods` type errors
    - [ ] `{accounts,chain,profile-sync}-controller` as dependencies
- `@metamask/providers` update to `^17.1.0` is being 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] Fix: MetaMask/providers#347

## Changelog

### `@metamask/snaps-cli` 

```md
### Changed
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](#2445))

### Fixed
- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#2445](#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`

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

### `@metamask/snaps-execution-environments`

```md
### Changed
- Bump `@metamask/rpc-errors` from `^6.2.1` to `^6.3.1` ([#2445](#2445))
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](#2445))

### Fixed
- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#2445](#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](#2445))
  - `17.1.0` and `17.1.1` introduce regressions.
```

### `@metamask/snaps-jest`

```md
### Changed
- Bump `@metamask/base-controller` from `^6.0.0` to `^6.0.1` ([#2445](#2445))
- Bump `@metamask/key-tree` from `^9.1.1` to `^9.1.2` ([#2445](#2445))
- Bump `@metamask/permission-controller` from `^10.0.0` to `^10.0.1` ([#2445](#2445))
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](#2445))

### Fixed
- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#2445](#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`

```md
### Changed
- Bump `@metamask/key-tree` from `^9.1.1` to `^9.1.2` ([#2445](#2445))
- Bump `@metamask/permission-controller` from `^10.0.0` to `^10.0.1` ([#2445](#2445))
- Bump `@metamask/rpc-errors` from `^6.2.1` to `^6.3.1` ([#2445](#2445))
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](#2445))

### Fixed
- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#2445](#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`

```md
### Changed
- Bump `@metamask/key-tree` from `^9.1.1` to `^9.1.2` ([#2445](#2445))
- Bump `@metamask/rpc-errors` from `^6.2.1` to `^6.3.1` ([#2445](#2445))
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](#2445))

### Fixed
- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#2445](#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](#2445))
  - `17.1.0` and `17.1.1` introduce regressions.
```

### `@metamask/snaps-simulator` (major)

```md
### Changed
- **BREAKING:** Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](#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](#2445))
- Bump `@metamask/key-tree` from `^9.1.1` to `^9.1.2` ([#2445](#2445))
- Bump `@metamask/permission-controller` from `^10.0.0` to `^10.0.1` ([#2445](#2445))

### Fixed
- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#2445](#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`

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

### Fixed
- Replace `superstruct` with ESM-compatible `@metamask/superstruct` `^3.1.0` ([#2445](#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`

```md
### Changed
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](#2445))
```

### `@metamask/test-snaps`

```md
### Changed
- Bump `@metamask/utils` from `^8.3.0` to `^9.1.0` ([#2445](#2445))

### Fixed
- Set `@metamask/providers` from `^17.0.0` to `17.0.0` ([#2445](#2445))
  - `17.1.0` and `17.1.1` introduce regressions.
```
@MajorLift MajorLift force-pushed the 231208-typescript-module-resolution branch 2 times, most recently from c4cc6dd to 9ebfa84 Compare July 19, 2024 01:57
…7.8.0`, `@metamask/snaps-controllers` to `^9.3.0`
@MajorLift MajorLift force-pushed the 231208-typescript-module-resolution branch from 9ebfa84 to 59fe394 Compare July 19, 2024 17:02
@MajorLift MajorLift force-pushed the 231208-typescript-module-resolution branch from c273024 to 6b0075d Compare July 19, 2024 17:05
@MajorLift MajorLift force-pushed the 231208-typescript-module-resolution branch from 986f16f to f50b209 Compare July 19, 2024 18:03
@MajorLift MajorLift requested a review from a team July 19, 2024 18:11
Copy link
Contributor Author

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

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

Notes for reviewers

@@ -245,6 +244,7 @@ export function getIpfsCIDv1AndPath(ipfsUrl: string): {
const cid = index !== -1 ? url.substring(0, index) : url;
const path = index !== -1 ? url.substring(index) : undefined;

const { CID } = await import('multiformats');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

multiformats is an ESM-only package, and we are replacing its import statement with dynamic import syntax.

This causes the following changes:

  • getIpfsCIDv1AndPath, getFormattedIpfsUrl are now async functions (this is a breaking change for assets-controllers).
  • The --experimental-vm-modules flag needs to be prepended to our build scripts that run jest, with accompanying updates to our yarn constraints.

Copy link
Member

Choose a reason for hiding this comment

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

How did this work previously if it's ESM-only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So far as I can tell, our previous moduleResolution option node/node10 will happily pass ESM-only packages into require calls without raising any warnings.

So it's likely the warnings we're now seeing with node16 should have been there all along. The package migrated to ESM 4 years ago (multiformats/js-multiformats@f925195#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519).

At runtime, either the module was interoperable by coincidence (not quite sure if this is possible or likely -- maybe if there's no ESM-specific syntax?), or it was breaking and we didn't notice.

@@ -312,19 +312,19 @@ gen_enforced_field(WorkspaceCwd, 'scripts.changelog:update', CorrectChangelogUpd
\+ atom_concat(ExpectedPrefix, _, ChangelogUpdateCommand).

% All non-root packages must have the same "test" script.
gen_enforced_field(WorkspaceCwd, 'scripts.test', 'jest --reporters=jest-silent-reporter') :-
gen_enforced_field(WorkspaceCwd, 'scripts.test', 'NODE_OPTIONS=--experimental-vm-modules jest --reporters=jest-silent-reporter') :-
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The --experimental-vm-modules flag is necessary if there are dynamic imports in the compiled code used by our tests.

Comment on lines +1 to +9
export type { AutoManagedNetworkClient } from './create-auto-managed-network-client';
export * from './NetworkController';
export * from './constants';
export type { BlockTracker, Provider } from './types';
export type { NetworkClientConfiguration } from './types';
export type {
NetworkClientConfiguration,
InfuraNetworkClientConfiguration,
CustomNetworkClientConfiguration,
} from './types';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These exports accommodate the new import statements in Token{Detection,Rate}Controller.test.ts.

Previously, these types were accessed through subpath imports, which are now not supported unless specified in the library's package.json exports field.

@@ -12,6 +12,7 @@ ignores:
- '@metamask/create-release-branch'
- 'depcheck'
- 'eslint-interactive'
- 'rimraf'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only listed as a devDep in the root package.json, where it's used in a build script but not in any modules.

import type { Hex } from '@metamask/utils';
import { add0x } from '@metamask/utils';
import assert from 'assert';
import nock from 'nock';
import { useFakeTimers } from 'sinon';

import { advanceTime } from '../../../tests/helpers';
import { createMockInternalAccount } from '../../accounts-controller/src/tests/mocks';
Copy link
Contributor Author

@MajorLift MajorLift Jul 19, 2024

Choose a reason for hiding this comment

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

With the Node{16,Next} option, subpath imports are no longer supported unless explicitly defined in the source package manifest's "exports" field.

Here, an import from @metamask/accounts-controller/src/tests/mocks needed to be replaced with a relative path import.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I'm not sure how this slipped through the cracks! Makes sense.

@@ -43,6 +43,7 @@
"pre-push": "yarn lint"
},
"resolutions": {
"@metamask/snaps-sdk@6.1.0/@metamask/providers": "17.1.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +10 to +16
### 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](https://github.com/MetaMask/snaps/pull/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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The packages affected by the yarn resolution are {accounts,chain,profile-sync}-controller. It's safe to temporarily merge the yarn resolution into main, as long as these packages aren't released (and even if they are @metamask/providers is not a direct dependency of any of these packages).

In any case, these three packages will be excluded from the core releases that follow this PR, since they are blocked by a snaps-controllers release as well.

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

I obviously underestimated how long this would take. Fantastic work on keeping track and staying on top of all of the downstream upgrades in order to unlock this PR 🎉

clap

@@ -70,7 +70,7 @@
"@metamask/auto-changelog": "^3.4.4",
"@metamask/eth-json-rpc-provider": "^4.1.1",
"@metamask/ethjs-provider-http": "^0.3.0",
"@metamask/keyring-api": "^8.0.0",
"@metamask/keyring-api": "^8.0.1",
Copy link
Contributor Author

@MajorLift MajorLift Jul 19, 2024

Choose a reason for hiding this comment

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

For the @MetaMask/confirmations team: this is the only change in this PR to a confirmations-owned package (aside from the typescript bumps and test build script updates on every package).

@MajorLift MajorLift merged commit 668e96a into main Jul 22, 2024
116 checks passed
@MajorLift MajorLift deleted the 231208-typescript-module-resolution branch July 22, 2024 13:17
MajorLift added a commit that referenced this pull request Jul 23, 2024
…tion for `@metamask/providers` (#4547)

## References

- Contributes to #3651
- Follows from #3645

## Changelog

### `@metamask/accounts-controller`

```md
### Changed

- Bump `@metamask/snaps-sdk` from `^6.1.0` to `^6.1.1` ([#4547](#4547))
- Bump `@metamask/snaps-utils` from `^7.8.0` to `^7.8.1` ([#4547](#4547))
```

### `@metamask/chain-controller`

```md
### Changed

- Bump `@metamask/snaps-controllers` from `^9.3.0` to `^9.3.1` ([#4547](#4547))
- Bump `@metamask/snaps-sdk` from `^6.1.0` to `^6.1.1` ([#4547](#4547))
- Bump `@metamask/snaps-utils` from `^7.8.0` to `^7.8.1` ([#4547](#4547))
```

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

```md
### Changed

- Bump `@metamask/snaps-controllers` from `^9.3.0` to `^9.3.1` ([#4547](#4547))
- Bump `@metamask/snaps-sdk` from `^6.1.0` to `^6.1.1` ([#4547](#4547))
- Bump `@metamask/snaps-utils` from `^7.8.0` to `^7.8.1` ([#4547](#4547))
```

## 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
@Gudahtt Gudahtt mentioned this pull request Jul 26, 2024
3 tasks
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.

4 participants