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

Migrate libraries into core monorepo #1079

Open
3 of 25 tasks
Gudahtt opened this issue Jan 25, 2023 · 4 comments
Open
3 of 25 tasks

Migrate libraries into core monorepo #1079

Gudahtt opened this issue Jan 25, 2023 · 4 comments
Labels

Comments

@Gudahtt
Copy link
Member

Gudahtt commented Jan 25, 2023

We would like to migrate most MetaMask libraries into this monorepo to make them easier to maintain. We may want to exclude libraries that are:

  • Experimental
  • Deprecated, or not encouraged to be used in our codebase anymore
  • Not used directly in a MetaMask client
  • Intended primarily for external developers integrating with MetaMask
  • Specific to extensions or React Native
  • Too noisy/high-volume
  • Deployed both as a website and an npm package
  • Controllers (these are represented by Merge extension and mobile controllers #700)
  • Conceptually distinct in some way (this one is subjective)

Here is the list of libraries that seem like a good fit today:

See #1079 (comment) for instructions on accomplishing this.

@mcmire
Copy link
Contributor

mcmire commented Jul 24, 2023

I propose we focus on the following libraries for Q3:

  • @metamask/utils
  • @metamask/json-rpc-engine
  • @metamask/eth-json-rpc-provider (not in the list above because it was created after the above list was built)
  • @metamask/json-rpc-middleware-stream

@mcmire
Copy link
Contributor

mcmire commented Aug 31, 2023

I went through the list of existing repos and found some that were omitted from the list above:

  • browser-passworder — Is this because it's extension only?
  • Snap-related libraries (eth-snap-keyring, keyring-snaps-registry, snap-simple-keyring, snaps-registry) — is this because Snaps owns these?
  • object-multiplex (used by providers) — Is it because it's not used directly by the wallet?
  • post-message-stream — Is this because it's extension only?
  • ppom-validator — Is this because it's extension only?
  • rpc-errors, safe-event-emitter, eth-json-rpc-provider, key-tree, swappable-obj-proxy — Are these missing because they were created after the list above was compiled?
  • web3-stream-provider — Is this because it's extension only?

@mcmire
Copy link
Contributor

mcmire commented Aug 31, 2023

UPDATE October 17, 2023:

Before starting work on future library migrations, please make sure to consult @MajorLift's migration checklist here. The instructions here only cover step B.0 in this checklist.


One thing we want to absolutely make sure to do when we migrate a library to this repo is to preserve all of the Git history.

I experimented with this using @metamask/utils and came up with a process. At a high level, we need to:

  1. Clone the repo for the library somewhere.
  2. Rewrite the entire history of the library as though the entire project had been located in a merged-packages/<library-name> directory all along. (You will soon see why.)
  3. Navigate to core.
  4. Add the library repo as a (local) remote, and fetch its history.
  5. Create a merge commit that connects the primary branch of the library with core's main branch (ignoring unrelated history).
  6. Done! Now we can submit a pull request.

Now we see why we performed step 2: because we want the library to initially be located in core under merged-packages/<library-name>. This directory is special in that ESLint and Prettier will ignore it. This allows us to follow up with another pull request that moves the library into packages and incorporate it into the release workflow.

Here is a list of detailed steps:

  1. Install git-filter-repo. This tool is like Git's filter-branch command, but much easier to use and much less error-prone.
  2. Navigate to a temporary directory: cd /tmp
  3. Clone the repo for the library you want to migrate: git clone https://github.com/MetaMask/<repo-name> (e.g. <repo-name> = utils). Do NOT use an existing clone.
  4. cd into the newly cloned repo.
  5. Run git filter-repo --to-subdirectory-filter merged-packages/<package-name> (e.g. <package-name> = utils). This will rewrite all history to move all files to merged-packages/<package-name>. (This is why we're making a fresh clone in a temporary directory — this action is irreversible.)
  6. cd to wherever you keep the core repo on your machine.
  7. Add the library as a remote: git remote add <package-name> /tmp/<package-name>.
  8. Fetch history: git fetch <package-name> --no-tags
  9. Make a new branch: git checkout -b migrate-<package-name>
  10. Merge the library into the monorepo: git merge --allow-unrelated-histories <package-name>/<primary-branch> (e.g. <primary-branch> = main)
  11. Make a pull request.

The resulting pull request should look like this: #1520

MajorLift added a commit that referenced this issue Oct 2, 2023
## Explanation

This is the first of two PRs that will migrate
[`eth-json-rpc-provider`](https://github.com/MetaMask/eth-json-rpc-provider)
into the core monorepo by following these steps:
#1079 (comment).

- This PR focuses on migrating the git history of the original package. 
- `eth-json-rpc-provider` is moved into a temporary directory in core
(`merged-packages/`) with git history fully intact.
- The second PR will remove the `merged-packages/` directory and
integrate `eth-json-rpc-provider` into the `packages/` directory,
resolving any issues arising from applying core monorepo
linter/compiler/build settings to the newly added package.
  
## Blocked by
- [x] MetaMask/eth-json-rpc-provider#29

## References

- Partially implements #1551
- See MetaMask/eth-json-rpc-provider#28 for
preparatory steps taken in original repo.

## Changelog

N/A since `@metamask/eth-json-rpc-provider` isn't usable from the
monorepo as of this PR.

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

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Co-authored-by: Alex Donesky <alex.donesky@consensys.net>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
Co-authored-by: legobt <6wbvkn0j@anonaddy.me>
Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
MajorLift added a commit that referenced this issue Oct 5, 2023
…on-rpc-provider" (#1790)

## Explanation

- The contents of `merged-packages/` shouldn't be altered, as it's a
temporary directory used for migrating packages into the monorepo.
- Any dependency updates for the migration target package need to be
handled manually. The migration process involves aligning dependency
versions with the monorepo (which may not use the latest version),
removing dependencies that are defined in the monorepo root, and
removing the `yarn.lock` file from the migration target subpackage.

## Reference

- Blocks #1764
- See
#1079 (comment) for
context on the role of `merged-packages/`.
MajorLift added a commit that referenced this issue Oct 11, 2023
…on-rpc-provider" (#1790)

## Explanation

- The contents of `merged-packages/` shouldn't be altered, as it's a
temporary directory used for migrating packages into the monorepo.
- Any dependency updates for the migration target package need to be
handled manually. The migration process involves aligning dependency
versions with the monorepo (which may not use the latest version),
removing dependencies that are defined in the monorepo root, and
removing the `yarn.lock` file from the migration target subpackage.

## Reference

- Blocks #1764
- See
#1079 (comment) for
context on the role of `merged-packages/`.
MajorLift added a commit that referenced this issue Oct 12, 2023
…on-rpc-provider" (#1790)

## Explanation

- The contents of `merged-packages/` shouldn't be altered, as it's a
temporary directory used for migrating packages into the monorepo.
- Any dependency updates for the migration target package need to be
handled manually. The migration process involves aligning dependency
versions with the monorepo (which may not use the latest version),
removing dependencies that are defined in the monorepo root, and
removing the `yarn.lock` file from the migration target subpackage.

## Reference

- Blocks #1764
- See
#1079 (comment) for
context on the role of `merged-packages/`.
@desi
Copy link
Contributor

desi commented Nov 20, 2023

browser-passworder, post-message-stream, web3-stream-provider — Not included because it's extension only
Snap-related libraries (eth-snap-keyring, keyring-snaps-registry, snap-simple-keyring, snaps-registry) —not included because Snaps only.
object-multiplex, swappable-obj-proxy, and safe-event-emitter - Not included because its not really wallet related, they are more general purpose
ppom-validator — Team decided to keep out of core for now - maybe revisit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants