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

Build - refactor background process to use html #10769

Merged
merged 5 commits into from
Mar 31, 2021
Merged

Build - refactor background process to use html #10769

merged 5 commits into from
Mar 31, 2021

Conversation

kumavis
Copy link
Member

@kumavis kumavis commented Mar 30, 2021

aligns background process entry point to match format of the other entry points (ui/home, ui/popup, ...). this will enable the "flat file" build refactor

additonally, instead of logic for adding/removing scripts from the build, i opted for copying empty files to replace the expected files

@kumavis kumavis requested a review from a team as a code owner March 30, 2021 09:40
@kumavis kumavis requested a review from ryanml March 30, 2021 09:40
@metamaskbot
Copy link
Collaborator

Builds ready [b3cda44]
Page Load Metrics (506 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint427853105
domContentLoaded28860150411053
load28960250611053
domInteractive28860150411053

Gudahtt
Gudahtt previously approved these changes Mar 31, 2021
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM! Made a couple of corrections to comments, but aside from that everything seems good.

@@ -80,13 +80,34 @@ const copyTargetsDev = [
pattern: '/chromereload.js',
dest: ``,
},
// empty files to suppress missing file errors
Copy link
Member

Choose a reason for hiding this comment

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

Huh, that's an interesting idea!

I had been meaning to address this warning by updating the build script to use these dependency bundles in development too, rather than just in prod. But I never got around to it. This seems like a simpler solution for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah this solution is sort of dumb and silly but saves a good chunk of code dedicated to conditionally flipping on and off script imports. simplicity won me over here.

ryanml
ryanml previously approved these changes Mar 31, 2021
Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

++ (discussed on Discord, but for now the empty.js file workaround is preferable to the alternative, which can be accomplished in a separate task)

@Gudahtt left some nits, but ran through smoke tests on both dev and prod builds and all looked good to me!

Co-authored-by: Mark Stacey <markjstacey@gmail.com>
@kumavis kumavis dismissed stale reviews from ryanml and Gudahtt via 529520d March 31, 2021 02:10
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
@metamaskbot
Copy link
Collaborator

Builds ready [6e49fcc]
Page Load Metrics (575 ± 43 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint45785894
domContentLoaded3936975738842
load3956985758943
domInteractive3936965728842

@kumavis kumavis merged commit 952adbc into develop Mar 31, 2021
@kumavis kumavis deleted the build-sys-6 branch March 31, 2021 03:12
@github-actions github-actions bot locked and limited conversation to collaborators Mar 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants