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 to new controller packages #5270

Merged
merged 10 commits into from Jan 10, 2023
Merged

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Nov 18, 2022

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

@metamask/controllers is deprecated, and most of the controllers that lived here are now located in their own package. This commit replaces @metamask/controllers in package.json with references to these packages and updates import lines to match.

Issue

Testing notes

This is a behind-the-scenes change, and there should be no changes to existing functionality — everything should work as it does currently. I'm not sure what the best protocol to test these sorts of changes is (maybe run through the most common scenarios?)

Custom registry setup (skip these steps, these were only relevant before we published these packages)

Note that before this PR gets merged, preview versions of the new packages, which live on GitHub Packages, will be used. Without specially configuring NPM, these packages will not be accessible locally, and therefore you will not be able to run yarn setup. To get this working:

  • Go into the token settings for your GitHub account and, if you have not already done so, create a classic personal access token. Make sure to store your token somewhere apart from GitHub as you won't be able to see it there.
  • Back in extension, create a file .npmrc in the root directory with the following content (note: this file is gitignored, so don't worry about it ending up in the repo):
    @metamask:registry=https://npm.pkg.github.com
    
    //npm.pkg.github.com/:_authToken=<insert your personal access token here>
    

Now you should be able to run yarn setup, then load the app as you normally would.

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@github-actions
Copy link
Contributor

github-actions bot commented Nov 18, 2022

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@mcmire mcmire added the DO-NOT-MERGE Pull requests that should not be merged label Nov 18, 2022
@mcmire
Copy link
Contributor Author

mcmire commented Nov 18, 2022

I have read the CLA Document and I hereby sign the CLA

.gitignore Show resolved Hide resolved
@mcmire
Copy link
Contributor Author

mcmire commented Nov 18, 2022

Marking as DONOTMERGE as we need to release 1.0.0 versions of the new packages, but before we can do that we need to ensure that extension also works with these changes. Open for review though, do your worst :)

@Gudahtt
Copy link
Member

Gudahtt commented Nov 18, 2022

I have added the token as PACKAGE_READ_TOKEN. I will look at updated CI to use it next

@mcmire
Copy link
Contributor Author

mcmire commented Nov 18, 2022

I'm going to keep this PR in draft mode so that if we need to make updates we can continue to access the preview versions of the aforementioned packages via the GitHub Package Registry, but feel free to leave your comments on this PR.

package.json Outdated Show resolved Hide resolved
@mcmire mcmire added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) release-5.12.0 labels Nov 18, 2022
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@Gudahtt Gudahtt force-pushed the migrate-to-new-controller-packages branch from d874a30 to a53a1e4 Compare November 22, 2022 16:16
@Gudahtt

This comment was marked as resolved.

@Gudahtt

This comment was marked as resolved.

@mcmire

This comment was marked as resolved.

@Gudahtt Gudahtt force-pushed the migrate-to-new-controller-packages branch from f565e97 to 39f804e Compare November 23, 2022 13:38
@Gudahtt

This comment was marked as resolved.

@Gudahtt Gudahtt force-pushed the migrate-to-new-controller-packages branch from 39f804e to bb172c3 Compare December 8, 2022 22:52
Base automatically changed from update/network-contoller to main December 14, 2022 01:28
@Gudahtt Gudahtt force-pushed the migrate-to-new-controller-packages branch from bb172c3 to faa0eee Compare December 14, 2022 01:39
@Gudahtt Gudahtt removed the DO-NOT-MERGE Pull requests that should not be merged label Dec 14, 2022
@Gudahtt Gudahtt force-pushed the migrate-to-new-controller-packages branch 3 times, most recently from 668be34 to e1675d1 Compare December 21, 2022 01:13
@Gudahtt Gudahtt marked this pull request as ready for review December 21, 2022 12:40
@Gudahtt Gudahtt requested a review from a team as a code owner December 21, 2022 12:40
@Gudahtt Gudahtt force-pushed the migrate-to-new-controller-packages branch from e1675d1 to 4cb3033 Compare December 21, 2022 21:19
mcmire and others added 9 commits January 5, 2023 17:09
`@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
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.
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
@Gudahtt Gudahtt force-pushed the migrate-to-new-controller-packages branch from 4cb3033 to 1e59d83 Compare January 5, 2023 20:46
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

LGTM. The changes are straightforward 👍

@gantunesr gantunesr removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 10, 2023
…ller-packages

* origin/main:
  [IMPROVEMENT] Remove deeplink warning for SDK and SDK as dependency (#5449)
  On-Ramp: Settings and Activation Keys (#5455)
  [FIX] Screenshot deterrent analytics (#5468)
  On-ramp: Remove unused constants (#5462)
  On-ramp: Refactor Regions view to componentization (#5413)
  Approval error when insufficient balance  (#5440)
  remove extra zero balance account potentially created from seeking ahead (#5459)
  [IMPROVEMENT] Refactor Analytics Events (#5454)
@Gudahtt Gudahtt added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Jan 10, 2023
Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

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

Pass automation. LGTM.

@Gudahtt Gudahtt merged commit 5b62796 into main Jan 10, 2023
@Gudahtt Gudahtt deleted the migrate-to-new-controller-packages branch January 10, 2023 18:34
@github-actions github-actions bot locked and limited conversation to collaborators Jan 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-qa Any New Features that needs a full manual QA prior to being added to a release. release-5.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants