refactor: Refactor how manifest flags are set #28686
Merged
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
Gudahtt
force-pushed
the
refactor-manifest-flag-logic
branch
2 times, most recently
from
f946a57 to
ae35e76
Compare
Gudahtt
changed the title
|
This refactor is in support of #28687 |
7 tasks
Gudahtt
force-pushed
the
refactor-manifest-flag-logic
branch
from
ae35e76 to
625f942
Compare
Gudahtt
force-pushed
the
refactor-manifest-flag-logic
branch
3 times, most recently
from
8899d67 to
2287421
Compare
Builds ready [8899d67]
Page Load Metrics (2248 ± 87 ms)
Bundle size diffs
|
Builds ready [2287421]
Page Load Metrics (1785 ± 136 ms)
Bundle size diffs
|
Builds ready [2f7df84]
Page Load Metrics (1726 ± 94 ms)
Bundle size diffs
|
The pieces of `set-manifest-flags.ts` related to _getting_ flags have been moved to the new `get-manifest-flags.ts` module. This module will be used in a later PR by a script run during a CI workflow. The migrated steps were also made asynchronous so that the asynchronous steps could be run in parallel rather than blocking the process. Documentation has been added to the `ManifestFlags` type as well, in preparation for adding a new property in the next PR.
Co-authored-by: Howard Braham <[email protected]>
Co-authored-by: Howard Braham <[email protected]>
Gudahtt
force-pushed
the
refactor-manifest-flag-logic
branch
from
f28b0ac to
4b330e6
Compare
HowardBraham
previously approved these changes
Nov 28, 2024
Builds ready [afda5dd]
Page Load Metrics (2144 ± 133 ms)
Bundle size diffs
|
development/lib/get-manifest-flag.ts
Outdated
| * | ||
| * @returns Any manifest flags found | ||
| */ | ||
| export async function getManifestFlags(): Promise<ManifestFlags> { |
There was a problem hiding this comment.
We now have two functions in the code base named getManifestFlags that both end up getting the same flags, but in different ways (runtime vs compile time) :-/ One is async (this one), and the other must stay synchronous as it is designed to be blocking and used on app initialization.
Think you could rename one or the other?
There was a problem hiding this comment.
Good point. Seems better to rename this one. Maybe fetchManifestFlags or createManifestFlags?
There was a problem hiding this comment.
Either is fine with me!
There was a problem hiding this comment.
@Gudahtt @davidmurdoch @DDDDDanica I think this one should be called fetchManifestFlagsFromGitAndPR
|
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. |
Builds ready [77fcfbf]
Page Load Metrics (1675 ± 59 ms)
Bundle size diffs
|
HowardBraham
previously approved these changes
Dec 12, 2024
HowardBraham
previously approved these changes
Dec 12, 2024
HowardBraham
force-pushed
the
refactor-manifest-flag-logic
branch
from
4a2f700 to
9af5440
Compare
HowardBraham
approved these changes
Dec 12, 2024
DDDDDanica
approved these changes
Dec 12, 2024
|
LGTM ! |
Builds ready [9af5440]
Page Load Metrics (1624 ± 67 ms)
Bundle size diffs
|
davidmurdoch
approved these changes
Dec 12, 2024
Gudahtt
commented
Dec 12, 2024
metamaskbot
added
the
release-12.10.1
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
The pieces of
set-manifest-flags.tsrelated to getting flags have been moved to the newget-manifest-flags.tsmodule. This module will be used in a later PR by a script run during a CI workflow.The migrated steps were also made asynchronous so that the asynchronous steps could be run in parallel rather than blocking the process.
Documentation has been added to the
ManifestFlagstype as well, in preparation for adding a new property in the next PR.Related issues
Related to #28685
Manual testing steps
N/A, no functional changes.
Screenshots/Recordings
N/A
Pre-merge author checklist
Pre-merge reviewer checklist