chore: integrate @metamask/utils into packages/ - #10185
Conversation
@metamask/utils into packages/
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Caution MetaMask internal reviewing guidelines:
|
949785c to
0a5e81f
Compare
| "@metamask/network-controller": "^37.0.0", | ||
| "@metamask/superstruct": "^3.4.1", | ||
| "@metamask/utils": "^11.12.0", | ||
| "@metamask/utils": "^12.0.0", |
There was a problem hiding this comment.
Hmm, this bumps to 12.0.0 at the same time. Should we do that in a separate PR?
There was a problem hiding this comment.
Yes because the local version is 12 now
There was a problem hiding this comment.
I am investigating the split... as it looks like the scope did grow up
There was a problem hiding this comment.
@mcmire this has now been splited into two PRs stacked
| "test:verbose": "NODE_OPTIONS=--experimental-vm-modules jest --verbose", | ||
| "test": "yarn test:unit && yarn test:types", | ||
| "test:clean": "yarn test:unit:clean && yarn test:types", | ||
| "test:types": "tstyche", |
There was a problem hiding this comment.
This bumps to tstyche at the same time. Should we do that in a separate PR?
There was a problem hiding this comment.
Normally yes, but tsd can't work here. The package test job runs yarn workspace <pkg> run test with no build step, and tsd requires a built typings file even though these tests import from source, so CI would fail.
It worked in the utils repo because test:types ran inside the build job right after yarn build, while the test job only ran test:source. Here both halves run in one job that never builds, and constraints mandate test = test:unit && test:types, so splitting them across jobs isn't an option either.
Doing the swap separately would also mean landing three tsd workarounds and reverting them right after: composite: false, --typings ./dist/index.d.ts, and excluding *.test-d.ts from tsconfig.lint.json.
I should have done this in the old utils repo, but I have missed this one
There was a problem hiding this comment.
Yeah in the future I think we should basically do everything we can in the source repo to match core before moving it over to core. I believe that's the intended goal of the package migration guide.
| "@typescript-eslint/naming-convention": { | ||
| "count": 1 | ||
| } | ||
| }, |
There was a problem hiding this comment.
Oof 😬 Maybe we should have another PR that fixes these type errors? Ideally we shouldn't introduce the package with a bunch of suppressions.
There was a problem hiding this comment.
Yes definitely I was planning to follow up on these, without growing the scope here
There was a problem hiding this comment.
Again, it might have been better to address these in the utils repo before moving over to core.
| @@ -5,7 +5,11 @@ | |||
| */ | |||
| "extends": "./tsconfig.base.json", | |||
| "compilerOptions": { | |||
| "noEmit": true | |||
| "noEmit": true, | |||
| "paths": { | |||
There was a problem hiding this comment.
Hmm, why is this necessary? TypeScript should be able to find the exports we've defined in utils's package.json.
There was a problem hiding this comment.
Because they are used by scripts, and otherwise lint:teams was failing, it looks like tsconfig.scripts.json is there but not used after all.
This configuration is intended for the scripts/ directory... It's currently not actually used for that purpose, but it will be in a future PR.
There was a problem hiding this comment.
Ah, strange, okay. It's okay for now. I wonder if a good way to fix this would be to make create-package a private package, then move all scripts that aren't create-package into a second private package. Then we "shouldn't" need a separate tsconfig.scripts.json. Maybe we can worry about that later though.
53f7cf3 to
e93a2b8
Compare
3718029 to
03b72a3
Compare
| "path": "../profile-sync-controller/tsconfig.build.json" | ||
| }, | ||
| { | ||
| "path": "../utils/tsconfig.build.json" |
There was a problem hiding this comment.
We don't need to modify the tsconfig.json and tsconfig.build.json across packages until we upgrade packages to 12.0.0, right?
There was a problem hiding this comment.
Oh, hmm, it seems that lint:tsconfigs doesn't realize that we aren't depending on the workspace version of utils. Okay. This is alright I guess, there's no harm in including these.
There was a problem hiding this comment.
The docs are explicit on doing this in one PR https://github.com/MetaMask/core/blob/main/docs/processes/package-migration-process-guide.md#1-move-the-migration-target-directory-from-merged-packages-into-packages
The split though was done here because they would both be merged at once but the review will be a bit easier
There was a problem hiding this comment.
Oh I see, I forgot about that, thank you.
| "references": [] | ||
| "references": [ | ||
| { | ||
| "path": "../utils/tsconfig.lint.json" |
There was a problem hiding this comment.
Same goes for tsconfig.lint.json files — we shouldn't need to modify these yet right?
There was a problem hiding this comment.
Never mind, I guess this is necessary :)
7698097 to
dd04556
Compare
Step D-3 of the [package migration process](https://github.com/MetaMask/core/blob/main/docs/processes/package-migration-process-guide.md). `@metamask/utils` now lives in the core monorepo ([MetaMask/core#10185](MetaMask/core#10185) moved it, [MetaMask/core#10192](MetaMask/core#10192) bumped the dependents), so this swaps the "migration in progress" notice for the archived one from the guide. Open issues have been transferred to core and the remaining open PRs are commented and locked. Archiving the repo is the next step. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only README change with no runtime or dependency impact. > > **Overview** > Updates the **README** migration banner for package migration step D-3: it no longer warns that migration is in progress or asks contributors to reopen PRs in **core**, and instead states that **`@metamask/utils`** has moved to the [**MetaMask/core**](https://github.com/MetaMask/core) monorepo, **this repo is archived**, and all future work should happen in **core**. > > The notice is also reformatted from a single-line HTML table into a multi-line layout; installation and API content are unchanged. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 13e0cdd. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
## Explanation Follow up to MetaMask#10185, which moved `@metamask/utils` into `packages/`. This bumps its dependents to the local version. - `@metamask/utils` bumped from `^11.12.0` to `^12.0.0` in 79 packages and the root - Removes the temporary `@metamask/utils` entry from `ALLOWED_INCONSISTENT_DEPENDENCIES` in `yarn.config.cjs`, which MetaMask#10185 added so that constraints would pass while the dependents were still on the published version - Changelog entries recorded for the bump, plus the lockfile dedupe and README dependency graph that follow from it These two PRs are meant to land together, so the intermediate state where dependents still resolve the published copy never reaches `main`. Splitting it this way keeps MetaMask#10185 reviewable: the move, the config rewiring and the type test change are separate from 79 mechanical version bumps. ## References - Follows MetaMask#10185 ## 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 - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > A major-version bump of a shared utility touches most packages; runtime risk is low if v12 is compatible, but consumers and lingering v11 transitive deps (keyring) can cause type or duplicate-package issues until fully aligned. > > **Overview** > This PR aligns the monorepo with the in-repo **`@metamask/utils` v12** after it was moved into `packages/` (follow-up to MetaMask#10185). **79 packages** plus the root bump the dependency from **`^11.12.0` to `^12.0.0`**, with matching **Unreleased changelog** entries and an updated **README dependency graph**. > > Tooling is cleaned up so builds use the workspace package instead of the published copy: **`tsconfig.packages.json`** now maps `@metamask/utils` to local source (the temporary `node_modules` override is removed), and **`yarn.config.cjs`** drops the **`ALLOWED_INCONSISTENT_DEPENDENCIES`** exception for `@metamask/utils`. > > There are small TypeScript follow-ups from the version split: **`network-controller`** removes a stale `@ts-expect-error` on Infura middleware now that types match v12, while **`keyring-controller`** adds a documented `@ts-expect-error` when casting legacy keyrings until **`@metamask/keyring-utils`** also depends on v12. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 8bd8970. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Explanation
Phase C of migrating
@metamask/utils, following the package migration process guide. This moves the package frommerged-packages/intopackages/and wires it into the monorepo.merged-packages/utils→packages/utils, registered as a workspacetsconfig.json,tsconfig.build.jsonandtsconfig.lint.json@metamask/utils@^12.0.0#10192) to keep this diff reviewable. This PR adds a temporary@metamask/utilsentry toALLOWED_INCONSISTENT_DEPENDENCIESinyarn.config.cjsso constraints pass while dependents are still on the published version; chore: bump dependents to@metamask/utils@^12.0.0#10192 removes it. The two are meant to land together.yarn lint:tsconfigs:fix:all(174 files). Without themyarn lint:tscreported 536 errors; it is now cleanyarn constraints --fixapplied, which split the test scripts intotest:unitandtest:typescodeowners.tsandteams.json, assigning the package to@MetaMask/core-platformas with the other foundational packagesyarn readme-content:updateType tests moved from
tsdtotstycheThe package's type tests import from source (
./index.js,./misc.js), so they never needed built declarations.tsdrequires a typings file as a structural precondition regardless, which forced a build purely to satisfy the tool.tstychetype checks source directly and is already used by@metamask/messenger, so the four*.test-d.tsfiles are now*.tst.ts, with all 120 assertions preserved.That removed three workarounds: a
composite: falseoverride fortsd, a--typingsflag, and the*.test-d.tsexclusion intsconfig.lint.json. That last one existed becausetsd's own types pull inlib="esnext", widening built in types likeReadonlySetpast this package's target. With that gone the type tests are covered bylint:tscrather than excluded from it, andtsdleaves the lockfile entirely.Root tooling now resolves the package from source
Nine files at the repository root import
@metamask/utils, which previously arrived prebuilt from npm. As a workspace itsdistdoes not exist until something builds it, and the lint jobs do not build, soconstraints,lint:teamsandlint:tsconfigs:allall failed on a fresh checkout.Adding a
pathsentry to the roottsconfig.jsonfixes the scripts.yarn.config.cjsis the exception, since Yarn loads it as plain CommonJS wherepathsdo not apply and a.tsfile cannot be required. It only usedhasProperty, whose runtime body is a single expression, so it now callsObject.hasOwndirectly. That is available on every supported Node version.All of the following pass with no build artifacts present:
constraints,lint:teams,lint:tsconfigs:all,readme-content:check,codeowners:check,lint:tsc,lint:misc:check, and the package's own tests.Inherited lint findings
86 pre-existing findings in the package, mostly
no-explicit-anyin tests, are recorded ineslint-suppressions.jsonrather than fixed here, matching how this repo tracks that debt. They came from rules the package's own ESLint config had disabled before the migration.References
@metamask/utilsgit history intomerged-packages/#10175, chore: reset the migrated@metamask/utilschangelog #10178, chore: remove@metamask/utilsfiles owned by the monorepo root #10179, chore: extend@metamask/utilsconfigs from the monorepo root #10180, chore: align@metamask/utilsdeps and scripts with the monorepo #10181 and chore: update the@metamask/utilsREADME for the monorepo #10182docs/processes/package-migration-process-guide.mdChecklist
Note
Medium Risk
Wide monorepo integration touches build graphs and resolution for a foundational dependency; temporary dual resolution (workspace v12 vs published v11) must land with the follow-up bump to avoid inconsistent runtime/types.
Overview
@metamask/utilsis now a workspace package atpackages/utils(v12.0.0) instead of living undermerged-packages/or npm-only. Repo metadata, CODEOWNERS/teams.json, README list/graph, and roottsconfigproject references are updated so the package builds and lints with the rest of core.Monorepo wiring: Jest and root/script
tsconfigpaths resolve@metamask/utils(and/node) to TypeScript source so tests and lint work without a prior build. Packagetsconfigreferences to../utilswere added across dependents.tsconfig.packages.jsonstill maps@metamask/utilsto published v11 innode_modulesuntil a follow-up bumps dependents;yarn.config.cjstemporarily allows that version skew and drops the@metamask/utilsimport in favor ofObject.hasOwn.Package tooling: Type tests move from
tsd(*.test-d.ts) totstyche(*.tst.ts); build excludes*.tst.ts. Unused@metamask/utilsdependencies are removed from a few packages (e.g. messenger, ai-controllers). Pre-existing ESLint issues inpackages/utilsare recorded ineslint-suppressions.json. Stalemerged-packages/utilstype-test and lint config files are removed.Reviewed by Cursor Bugbot for commit dd04556. Bugbot is set up for automated code reviews on this repo. Configure here.