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

Remove 3box feature and delete ThreeBoxController #14571

Merged
merged 12 commits into from
Oct 31, 2022
Merged

Remove 3box feature and delete ThreeBoxController #14571

merged 12 commits into from
Oct 31, 2022

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Apr 29, 2022

Removes the 3box sync feature and deletes the ThreeBoxController. Adds a migration to ensure that the ThreeBoxController is removed from persisted state.

This shaves 600kb or 3.2% from our bundle (18.5mb -> 17.9mb).

@rekmarks rekmarks requested review from a team and kumavis as code owners April 29, 2022 16:34
@rekmarks rekmarks requested a review from adonesky1 April 29, 2022 16:34
@kumavis
Copy link
Member

kumavis commented Apr 29, 2022

👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍 👍

@metamaskbot
Copy link
Collaborator

Builds ready [8db9516]
Page Load Metrics (1831 ± 62 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint873601205727
domContentLoaded16552049181912359
load16552049183112862
domInteractive16552049181912359

highlights:

storybook

Copy link
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

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

Oddly enough somehow the messages from app/_locales/zh/messages.json didn't get removed; I'm still seeing syncWithThreeBox, syncWithThreeBoxDescription, and syncWithThreeBoxDisabled keys.

brad-decker
brad-decker previously approved these changes May 2, 2022
Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

Hero. 🙏🏻 Non blocking suggestion for metrics cleanliness.

app/scripts/controllers/metametrics.js Show resolved Hide resolved
shared/constants/metametrics.js Show resolved Hide resolved
@Gudahtt
Copy link
Member

Gudahtt commented May 3, 2022

We need a deprecation plan before we can merge this, including messaging to any existing users

@darkwing darkwing added the DO-NOT-MERGE Pull requests that should not be merged label May 5, 2022
@darkwing
Copy link
Contributor

darkwing commented May 5, 2022

Marking is DO-NOT-MERGE until we resolve Mark's comments.

@danjm danjm mentioned this pull request Jul 12, 2022
@segun segun added the MV3 label Jul 12, 2022
@segun segun mentioned this pull request Jul 14, 2022
6 tasks
@danjm danjm added the PR - P1 identifies PRs deemed priority for Extension team label Jul 20, 2022
@kumavis
Copy link
Member

kumavis commented Jul 21, 2022

@Gudahtt i would claim that we dont need a migration path bc the feature was only ever experimental. i agree that informing users that had it enabled is worth blocking on.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 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.

@legobeat
Copy link
Contributor

This resolves #10608

Lint locale messages

lavamoat policy updates
The 3Box user trait has been restored and hard-coded as `false`. This
ensures that users don't get stuck in our metrics as having this trait.

A deprecation comment has been left in various places for this trait.
@legobeat

This comment was marked as resolved.

@Gudahtt

This comment was marked as resolved.

@Gudahtt Gudahtt dismissed darkwing’s stale review October 31, 2022 15:01

An in-app deprecation warning has been in place for some time now, and warned of this feature being deprecated in "early October"

@rekmarks rekmarks removed the DO-NOT-MERGE Pull requests that should not be merged label Oct 31, 2022
@metamaskbot
Copy link
Collaborator

Builds ready [d47f42c]
Page Load Metrics (2300 ± 162 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint912402233498239
domContentLoaded163929222260313150
load165030582300337162
domInteractive163929222260313150

highlights:

storybook

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

LGTM

@Gudahtt Gudahtt merged commit a8c1756 into develop Oct 31, 2022
@Gudahtt Gudahtt deleted the delete-3box branch October 31, 2022 16:20
@github-actions github-actions bot locked and limited conversation to collaborators Oct 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
MV3 PR - P1 identifies PRs deemed priority for Extension team stability team-extension-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.