-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
Convert to monorepo #831
Merged
Merged
Convert to monorepo #831
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcmire
commented
May 17, 2022
Completed work: * Copy a few modules to `packages/` * Upgrade Yarn to v3 * Add Yarn workspaces * Add Yarn constraints for `package.json` in root and package * Reconfigure TypeScript support new structure * Reconfigure Jest to support new structure Remaining work: * TypeDoc — it doesn't seem to support TypeScript project references, so we may have to generate docs for each package individually or figure out something else * ESLint — the `node` and `import` plugins are tripping up on imports which are aliases — we either need to disable the appropriate rules or get the plugins to recognize `tsconfig.json` appropriately
Mrtenz
reviewed
May 18, 2022
The package manager used has been upgraded from Yarn v1 to Yarn v3. All of the steps in the migration guide [1] have been followed. Effectively everything should work the same way it did before. Closes #847 [1]: https://yarnpkg.com/getting-started/migration
The old `setup` script has been replaced with a Yarn v3 plugin that will automatically run `allow-scripts` after install. We can now use `yarn install` again as normal. This was done to address a problem that happened when running `yarn setup` for the first time. If Yarn 3 wasn't installed already, it would throw an error. Yarn 3 doesn't automatically install itself until you run the `install` command directly. The `setup` script has been left behind for now so that it still works if people run it accidentally. We can remove it later once we've become accustomed to this new workflow.
Co-authored-by: Elliot Winkler <[email protected]>
Other changes: * Remove overrides in `.eslintrc` for import/*` rules * gitignore `*.tsbuildinfo` files * Fix file name of `constraints.pro` so it actually takes effect * Get `yarn lint` working
Pushed up some more fixes. Outstanding issues remaining:
|
|
Gudahtt
previously approved these changes
Nov 11, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! A couple of small things left to improve but nothing major
Gudahtt
approved these changes
Nov 11, 2022
Mrtenz
approved these changes
Nov 11, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Merged
mcmire
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Nov 16, 2022
`@metamask/controllers` is deprecated, and most of the controllers that lived here are now located in their own package ([1]). This commit replaces `@metamask/controllers` in `package.json` with references to these packages and updates `import` lines to match. [1]: MetaMask/core#831
8 tasks
mcmire
added a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Nov 18, 2022
`@metamask/controllers` is deprecated, and most of the controllers that lived here are now located in their own package ([1]). This commit replaces `@metamask/controllers` in `package.json` with references to these packages and updates `import` lines to match. [1]: MetaMask/core#831
3 tasks
Gudahtt
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Nov 22, 2022
`@metamask/controllers` is deprecated, and most of the controllers that lived here are now located in their own package ([1]). This commit replaces `@metamask/controllers` in `package.json` with references to these packages and updates `import` lines to match. [1]: MetaMask/core#831
Gudahtt
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Nov 23, 2022
`@metamask/controllers` is deprecated, and most of the controllers that lived here are now located in their own package ([1]). This commit replaces `@metamask/controllers` in `package.json` with references to these packages and updates `import` lines to match. [1]: MetaMask/core#831
Gudahtt
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Nov 24, 2022
* Migrate to new controller packages `@metamask/controllers` is deprecated, and most of the controllers that lived here are now located in their own package ([1]). This commit replaces `@metamask/controllers` in `package.json` with references to these packages and updates `import` lines to match. [1]: MetaMask/core#831 * Support GitHub registry for draft PRs (#16549) * Add additional allowed host to lockfile linter * Update LavaMoat policies * Add policy exception for nanoid * Add additional nanoid overrides * Update LavaMoat policies again * Bump controller packages * Update lavamoat * Bump controller packages * Update packages to v1.0.0 * Expand gitignore comment * Unpin controller dependencies, using ^ range instead Co-authored-by: Mark Stacey <[email protected]>
gantunesr
pushed a commit
that referenced
this pull request
Dec 8, 2022
Currently, this repo and everything within it is published as a single package. Using and maintaining this package, however, is problematic for a few reasons: 1. Even if your library uses a couple of controllers, you must add the entire package and all of its dependencies to your library's dependency tree. 2. Because this package is used by many teams, when we make a new release, even if that release contains changes to one controller, we must coordinate with all teams to ensure that nothing has broken. In addition, we want to be able to maintain our existing libraries more easily, as right now it is difficult due to code being split across multiple repositories. To solve this problem, this commit converts the existing structure to a monorepo structure, assigning controllers to packages which we can then publish individually. (A full list of packages is contained in the README.) Along with a monorepo structure comes with a litany of changes: * **TypeScript:** We have a "master" TypeScript config file, which is used by developers' code editors, but each package also has its own TypeScript config files. We are also using TypeScript project references, which allows us to inform TypeScript how all packages are connected to each other dependency-wise; this allows TypeScript to know which packages to build first. * **Jest:** Each package has its own Jest config file, and we use Yarn workspaces to run the tests for each package in parallel. * **ESLint:** We are able to lint the monorepo in the same way as we linted before. * **TypeDoc:** We've added TypeDoc to each package and use Yarn workspaces to generate docs for each package in parallel. * **Yarn:** A bunch of Yarn constraints have been added that verify that both the root package and each package has a well-formed `package.json`. * **Other notes:** * Some packages depend on other packages within the monorepo. In other words, we might have an import for `@metamask/base-controller` within a controller file. Out of the box both TypeScript and Jest won't know what to do with this. Although Yarn will add a symlink in `node_modules` to the proper directory in `packages` for the package in question, TypeScript expects the code for the package to be compiled (i.e. for `dist/` to be populated), and Jest, as it has custom resolver logic, doesn't know what to do at all. To make this possible we have to add a custom mapping for both TypeScript and Jest that will tell them what to do when it sees a `@metamask/*` import. * The GitHub Action workflow files have been standardized against the newest changes to the module template. Co-authored-by: Mark Stacey <[email protected]> Co-authored-by: Maarten Zuidhoorn <[email protected]>
Gudahtt
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Dec 8, 2022
`@metamask/controllers` is deprecated, and most of the controllers that lived here are now located in their own package ([1]). This commit replaces `@metamask/controllers` in `package.json` with references to these packages and updates `import` lines to match. [1]: MetaMask/core#831
Gudahtt
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Dec 14, 2022
`@metamask/controllers` is deprecated, and most of the controllers that lived here are now located in their own package ([1]). This commit replaces `@metamask/controllers` in `package.json` with references to these packages and updates `import` lines to match. [1]: MetaMask/core#831
Gudahtt
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Dec 21, 2022
`@metamask/controllers` is deprecated, and most of the controllers that lived here are now located in their own package ([1]). This commit replaces `@metamask/controllers` in `package.json` with references to these packages and updates `import` lines to match. [1]: MetaMask/core#831
Gudahtt
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Dec 21, 2022
`@metamask/controllers` is deprecated, and most of the controllers that lived here are now located in their own package ([1]). This commit replaces `@metamask/controllers` in `package.json` with references to these packages and updates `import` lines to match. [1]: MetaMask/core#831
Gudahtt
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Dec 21, 2022
`@metamask/controllers` is deprecated, and most of the controllers that lived here are now located in their own package ([1]). This commit replaces `@metamask/controllers` in `package.json` with references to these packages and updates `import` lines to match. [1]: MetaMask/core#831
Gudahtt
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Jan 5, 2023
`@metamask/controllers` is deprecated, and most of the controllers that lived here are now located in their own package ([1]). This commit replaces `@metamask/controllers` in `package.json` with references to these packages and updates `import` lines to match. [1]: MetaMask/core#831
Gudahtt
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Jan 10, 2023
* Migrate to new controller packages `@metamask/controllers` is deprecated, and most of the controllers that lived here are now located in their own package ([1]). This commit replaces `@metamask/controllers` in `package.json` with references to these packages and updates `import` lines to match. [1]: MetaMask/core#831 * Setup CI to download from GitHub Packages registry (#5274) CI now supports fetching packages from the GitHub Packages registry on draft PRs. This is meant to support the use of "preview" builds of controller packages, to more easily test controller changes before they are released.
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
Currently, this repo and everything within it is published as a single package. Using and maintaining this package, however, is problematic for a few reasons: 1. Even if your library uses a couple of controllers, you must add the entire package and all of its dependencies to your library's dependency tree. 2. Because this package is used by many teams, when we make a new release, even if that release contains changes to one controller, we must coordinate with all teams to ensure that nothing has broken. In addition, we want to be able to maintain our existing libraries more easily, as right now it is difficult due to code being split across multiple repositories. To solve this problem, this commit converts the existing structure to a monorepo structure, assigning controllers to packages which we can then publish individually. (A full list of packages is contained in the README.) Along with a monorepo structure comes with a litany of changes: * **TypeScript:** We have a "master" TypeScript config file, which is used by developers' code editors, but each package also has its own TypeScript config files. We are also using TypeScript project references, which allows us to inform TypeScript how all packages are connected to each other dependency-wise; this allows TypeScript to know which packages to build first. * **Jest:** Each package has its own Jest config file, and we use Yarn workspaces to run the tests for each package in parallel. * **ESLint:** We are able to lint the monorepo in the same way as we linted before. * **TypeDoc:** We've added TypeDoc to each package and use Yarn workspaces to generate docs for each package in parallel. * **Yarn:** A bunch of Yarn constraints have been added that verify that both the root package and each package has a well-formed `package.json`. * **Other notes:** * Some packages depend on other packages within the monorepo. In other words, we might have an import for `@metamask/base-controller` within a controller file. Out of the box both TypeScript and Jest won't know what to do with this. Although Yarn will add a symlink in `node_modules` to the proper directory in `packages` for the package in question, TypeScript expects the code for the package to be compiled (i.e. for `dist/` to be populated), and Jest, as it has custom resolver logic, doesn't know what to do at all. To make this possible we have to add a custom mapping for both TypeScript and Jest that will tell them what to do when it sees a `@metamask/*` import. * The GitHub Action workflow files have been standardized against the newest changes to the module template. Co-authored-by: Mark Stacey <[email protected]> Co-authored-by: Maarten Zuidhoorn <[email protected]>
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
Currently, this repo and everything within it is published as a single package. Using and maintaining this package, however, is problematic for a few reasons: 1. Even if your library uses a couple of controllers, you must add the entire package and all of its dependencies to your library's dependency tree. 2. Because this package is used by many teams, when we make a new release, even if that release contains changes to one controller, we must coordinate with all teams to ensure that nothing has broken. In addition, we want to be able to maintain our existing libraries more easily, as right now it is difficult due to code being split across multiple repositories. To solve this problem, this commit converts the existing structure to a monorepo structure, assigning controllers to packages which we can then publish individually. (A full list of packages is contained in the README.) Along with a monorepo structure comes with a litany of changes: * **TypeScript:** We have a "master" TypeScript config file, which is used by developers' code editors, but each package also has its own TypeScript config files. We are also using TypeScript project references, which allows us to inform TypeScript how all packages are connected to each other dependency-wise; this allows TypeScript to know which packages to build first. * **Jest:** Each package has its own Jest config file, and we use Yarn workspaces to run the tests for each package in parallel. * **ESLint:** We are able to lint the monorepo in the same way as we linted before. * **TypeDoc:** We've added TypeDoc to each package and use Yarn workspaces to generate docs for each package in parallel. * **Yarn:** A bunch of Yarn constraints have been added that verify that both the root package and each package has a well-formed `package.json`. * **Other notes:** * Some packages depend on other packages within the monorepo. In other words, we might have an import for `@metamask/base-controller` within a controller file. Out of the box both TypeScript and Jest won't know what to do with this. Although Yarn will add a symlink in `node_modules` to the proper directory in `packages` for the package in question, TypeScript expects the code for the package to be compiled (i.e. for `dist/` to be populated), and Jest, as it has custom resolver logic, doesn't know what to do at all. To make this possible we have to add a custom mapping for both TypeScript and Jest that will tell them what to do when it sees a `@metamask/*` import. * The GitHub Action workflow files have been standardized against the newest changes to the module template. Co-authored-by: Mark Stacey <[email protected]> Co-authored-by: Maarten Zuidhoorn <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remaining work
Look into why some tests involvingnot necessarybn.js
aren't working (see https://github.com/MetaMask/controllers/pull/831/files#r898282054)Completed work
packages/
packages/*
as workspacespackage.json
s../../scripts/*
)packages/controller-utils/src/util.ts
and extract any functions to their respective packagescustomExportConditions
instead oftransformIgnorePatterns
to use CJS exports from dependencies instead of ESM (see message in Slack). Shouldn't need to useallowJs
orts-jest/presets/js-with-ts
either.description
in each package'spackage.json
to match the READMEindex.ts
and make sure everything is being exported that should be, and nothing is being exported that shouldn'tname
of each package matches its directory and that itshomepage
matches thename
publish-release
step to take advantage of caching (Use cached dependencies and immutable install during publish #942)Deferred to a future PR
yarn workspaces foreach --topological-dev exec "echo '' >/dev/null"
to check for circular dependencies first--forceExit
*.tsbuildinfo
files in CI so thatyarn build
doesn't take as long for subsequent runscross-fetch
instead ofisomorphic-fetch
hexToText
@metamask/utils
fromHex
andtoHex
(or their equivalents) from@metamask/utils
@ts-expect-error
overany
How to review this PR
In addition to the comments I've left, I would check this branch out locally, and then:
yarn build
. This should compile the TypeScript files in each package and output them todist/
in the package's directory.yarn test
. This should run all tests for all packages.constraints.pro
. This allows us to make sure thatpackage.json
at the root and also at the per-package level have the right fields with the right values.tsconfig.json
. This makes use of project references to include all TypeScript code across all packages in a way that informs TypeScript which packages depend on which other packages. Per-package configuration is kept atpackages/*/tsconfig.json
, and all of these files inherit fromtsconfig.packages.json
.yarn build
,tsconfig.build.json
will be used instead. This also uses project references, but brings inpackages/*/tsconfig.build.json
for each package instead. All of these files inherit fromtsconfig.packages.build.json
and exclude test-specific files from the build. TypeScript will consult each package's TypeScript config to figure out where to output files (outDir
is relative to the package, not the root).jest.config.js
(previouslyjest.config.ts
), with common options extracted tojest.config.packages.js
in the root. The tests for each package are run individually. All tests are run usingyarn workspaces foreach
to iterate through all packages.We have configuredUpdate: It's true that we have dependencies that ship uncompiled but we've gotten around this by usingts-jest
to run not only TypeScript files but also JavaScript files through the TypeScript compiler (see thepreset
setting injest.config.packages.js
). This is necessary because there are some dependencies (multiformats
,ethjs-*
) which ship with uncompiled, and sometimes Babel-specific, code. I am not sure how we got around this before, but these files need to be transformed in order to be used at all.customExportConditions
in the Jest config file.The above constraint means that we need a specialUpdate: This is no longer the casetsconfig.test.json
for tests, which setsallowJs
totrue
. This means there are actually three levels of tsconfigs: dev, build, and test.