build: Add build type extensions #32724
Conversation
Builds ready [32a781a]
UI Startup Metrics (1208 ± 62 ms)
Benchmark value 23 exceeds gate value 18 for chrome browserify home p95 backgroundConnect Benchmark value 1659 exceeds gate value 1405 for firefox browserify home mean uiStartup Benchmark value 1478 exceeds gate value 1245 for firefox browserify home mean load Benchmark value 1477 exceeds gate value 1239 for firefox browserify home mean domContentLoaded Benchmark value 121 exceeds gate value 110 for firefox browserify home mean domInteractive Benchmark value 27 exceeds gate value 25 for firefox browserify home mean firstReactRender Benchmark value 15 exceeds gate value 11 for firefox browserify home mean getState Benchmark value 1457 exceeds gate value 1230 for firefox browserify home mean loadScripts Benchmark value 1988 exceeds gate value 1660 for firefox browserify home p95 uiStartup Benchmark value 1743 exceeds gate value 1495 for firefox browserify home p95 load Benchmark value 1743 exceeds gate value 1495 for firefox browserify home p95 domContentLoaded Benchmark value 1720 exceeds gate value 1475 for firefox browserify home p95 loadScripts Benchmark value 14 exceeds gate value 13 for firefox webpack home mean setupStore Sum of mean exceeds: 970ms | Sum of p95 exceeds: 1074ms Sum of all benchmark exceeds: 2044ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
@davidmurdoch how so? It's a single, battle-tested function that is good enough for production builds, and it performs more validation than the corresponding webpack implementation. If anything it will ensure that there's no drift between the build systems in their treatment of |
✨ Files requiring CODEOWNER review ✨🧩 @MetaMask/extension-devs
📜 @MetaMask/policy-reviewers Tip Follow the policy review process outlined in the LavaMoat Policy Review Process doc before expecting an approval from Policy Reviewers.
🔗 @MetaMask/supply-chain
|
rekmarks
force-pushed
the
rekm/build-type-extensions
branch
from
dc02323 to
2248525
Compare
Builds ready [2248525]
UI Startup Metrics (1214 ± 59 ms)
Benchmark value 34 exceeds gate value 33 for chrome browserify home p95 getState Benchmark value 2028 exceeds gate value 1935 for firefox webpack home p95 uiStartup Sum of mean exceeds: 0ms | Sum of p95 exceeds: 97ms Sum of all benchmark exceeds: 97ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
rekmarks
force-pushed
the
rekm/build-type-extensions
branch
from
2248525 to
b2fbbe8
Compare
rekmarks
force-pushed
the
rekm/build-type-extensions
branch
from
b2fbbe8 to
711f541
Compare
Builds ready [711f541]
UI Startup Metrics (1227 ± 71 ms)
Benchmark value 1068 exceeds gate value 1061 for chrome browserify home mean domContentLoaded Benchmark value 23 exceeds gate value 18 for chrome browserify home p95 backgroundConnect Benchmark value 11 exceeds gate value 9 for firefox browserify home mean setupStore Benchmark value 1677 exceeds gate value 1660 for firefox browserify home p95 uiStartup Benchmark value 1514 exceeds gate value 1495 for firefox browserify home p95 load Benchmark value 1513 exceeds gate value 1495 for firefox browserify home p95 domContentLoaded Benchmark value 27 exceeds gate value 26 for firefox webpack home mean backgroundConnect Benchmark value 1987 exceeds gate value 1935 for firefox webpack home p95 uiStartup Benchmark value 51 exceeds gate value 49 for firefox webpack home p95 backgroundConnect Sum of mean exceeds: 16ms | Sum of p95 exceeds: 113ms Sum of all benchmark exceeds: 129ms Bundle size diffs [🚀 Bundle size reduced!]
|
There was a problem hiding this comment.
@davidmurdoch how so? It's a single, battle-tested function that is good enough for production builds, and it performs more validation than the corresponding webpack implementation. If anything it will ensure that there's no drift between the build systems in their treatment of
builds.yml. I'll also type the function to avoidanypollution.
it performs more validation than the corresponding webpack implementation
Validation, especially at the cost of build performance, does not typically belong in the compilation/bundling functionality of our build system.
The yarn webpack command serves a couple of key goals:
-
Developer velocity - I benchmarked nearly every
import/requireand package, nearly every individual line and loop, minimized creation of poly/mega morphic object creation, and so much more. I didn't always choose the fastest way, since it is important we can maintain this code, but there are several hundred ~5-10ms "micro" optimizations in the new build system. Improving developer velocity is the reason ouryarn webpackcommand exists. -
Minimize scope - The new build system should never do testing, validation, or linting of the runtime code (runtime = both application and build system code) by default. Is
build.ymlpart of our runtime or is it part of compilation arguments? I could argue both ways, but I lean strongly towards it being part of the code for both our compiler/bundler and the runtime. Our compiler/bundler should validate user input (yarn webpack <options should be validated>), but not the runtime code. That should be part of a separate lint, validation, or testing process. -
Paradigm shift - Specifically, we want our compiler/bundler to be as declarative as feasible - our runtime code should instruct the build output as reasonably as it can - with instruction from the build config. Doing this will make it easier to switch build systems in the future (native options like
bun,rspack, andturbopackare on our radar), generally decreases the size and complexity of our compiler/bundler scripts, and forces more thought into how we do things (as opposed to the "ship now, fix later™" mindset I see here often). This means we attempt to minimize our scripts that manually pluck, move, and compile files, in favor of idiomatic solutions. -
Test coverage - The
./webpacktest directory has nearly 100% code coverage (cost/rewardwasn't enough to make it 100% at the time) -
TypeScript - We're a TypeScript repo with a very heavy JS problem. We can philosophize about this point later if you want.
-
Self contained - Why? Only because it helps support the priorities above. It is low on the list because, as you pointed out, we want to maintain effective output parity between the gulp+browserify and webpack systems, and our build config processing can be a bit complex, so utilizing some of the gulp-system's existing utils is currently necessary.
-
Production builds - I'd love for this to be higher on the list, but there are good reasons this isn't on our teams goals this quarter.
Regarding YAML, since it’s our chosen configuration format, we should leverage its built-in features, such as anchors and merge keys, for extend-like functionality rather than custom solutions like are being introduced in this PR. To do this in YAML 1.2 (our default, IIRC), enabling the merge feature in the parser options is required. If you've already evaluated built-in options and found they don't work for us it'd be great if you commented as to why.
On validation, what I ultimately want to move to is JSON Schema-based validation (for clarity: I'm not asking for this in this PR). Most IDEs offer plugins that do, which would allow us to see validation errors in our IDEs as well as use these same schemas for programmatic validation. We don't implement any JSON Schema-based YAML validation currently, but we do use custom IDE package.json validation.
To your point about preventing drift between build systems, while validation can ensure consistency, embedding it in the webpack-based compilation/bundling process sacrifices performance (if we validated everything -- I seriously doubt this one thing would be measurable), but most importantly, scope clarity. A better solution is to handle validation as an optional or separate step.
Ultimately, yarn webpack should be viewed as the "compilation" part of our build system; validation, linting, security analysis, testing, etc., belong elsewhere. A unified CLI might one day streamline these processes, but each component should remain focused on its specific role.
@davidmurdoch I'm not sure if we're on the same page about what this PR actually does. To begin, I wasn't aware of YAML's merge feature, but upon review, it unfortunately doesn't handle lists, so it doesn't meet my requirements. Now re: what I take to be your main point, this PR doesn't really interact with the compilation / bundling functionality of either of our build systems. To recap:
In other words, this PR is almost completely orthogonal to the new build system. Other tools that are part of our build system, e.g. webpack itself, validate their config files, so validating our own could hardly amount to a confusion of responsibilities. (Indeed, we were already validating the config file to some extent via Now, all of the above being the case, what are your concerns about the changes in this PR? |
I avoid using lists in config for this reason. When possible I prefer to use an object and then just take (and filter) values. Allows the overrider to override or values, but modifying order of values is less straight forward. |
Builds ready [c4ca53c]
UI Startup Metrics (1239 ± 72 ms)
Benchmark value 1081 exceeds gate value 1070 for chrome browserify home mean load Benchmark value 1074 exceeds gate value 1061 for chrome browserify home mean domContentLoaded Benchmark value 1378 exceeds gate value 1365 for chrome browserify home p95 uiStartup Benchmark value 25 exceeds gate value 18 for chrome browserify home p95 backgroundConnect Sum of mean exceeds: 30ms | Sum of p95 exceeds: 20ms Sum of all benchmark exceeds: 50ms Bundle size diffs [🚀 Bundle size reduced!]
|
|
@rekmarks Most of my response was to get you on the same page as me as to what our new build system intends to become: a system of discreet tools that work together. One of these tools is compilation/bundling. You had made a point that this PR is justified, in part, because it validates the YAML build file's output. In the context of the above, I was hoping to get you on the same page, in principle: validation should be a prerequisite to compilation. I didn't suspect that it would have a significant performance impact, but in the new system I think we should strive to compartmentalize the parts that make up the whole. So we are on the same page: validation is orthogonal to compilation ;-)
What the external YAML parser does is none of my business! haha :-) I'm not a stickler on this, I just want to make sure you really really want this in the new system.
I didn't know that. What a sad day! Perhaps we should switch to object/maps as @kumavis suggested (later, not suggesting you do it in this PR). It's a bummer that the YAML merge operator doesn't, uh, merge lists, seems like a very obvious use case 🙃. Sorry to send you on a fruitless investgation.
One of things I found surprising when I was building the webpack bundler process was that there are some util files in I know there is a lot to unpack in the list I shared above, but if you have disagreements with anything I went over I'd love to chat with you about it further (not in this PR, obviously). aside: one of these days I do hope to get this in as part of the lint process via JSON Schema validation. I'll do a proper review later (its after 7pm here), but some quick notes: if the JavaScript test file can be in TypeScript without having to refactor anything else that'd be great, and I wouldn't mind if you just re-exported |
Builds ready [bc19664]
UI Startup Metrics (1199 ± 62 ms)
Benchmark value 13 exceeds gate value 11 for firefox browserify home mean getState Benchmark value 11 exceeds gate value 9 for firefox browserify home mean setupStore Benchmark value 27 exceeds gate value 24 for firefox browserify home p95 getState Sum of mean exceeds: 4ms | Sum of p95 exceeds: 8ms Sum of all benchmark exceeds: 12ms Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [3d7c307]
UI Startup Metrics (1204 ± 61 ms)
Benchmark value 14 exceeds gate value 11 for firefox browserify home mean getState Benchmark value 28 exceeds gate value 24 for firefox browserify home p95 getState Sum of mean exceeds: 3ms | Sum of p95 exceeds: 113ms Sum of all benchmark exceeds: 116ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [80735b0]
UI Startup Metrics (1208 ± 58 ms)
Benchmark value 9 exceeds gate value 7 for chrome webpack home mean initialActions Benchmark value 39 exceeds gate value 32 for chrome webpack home mean setupStore Benchmark value 2554 exceeds gate value 2454 for chrome webpack home p95 uiStartup Benchmark value 285 exceeds gate value 65 for chrome webpack home p95 setupStore Benchmark value 39 exceeds gate value 38 for firefox webpack home mean firstReactRender Sum of mean exceeds: 10ms | Sum of p95 exceeds: 325ms Sum of all benchmark exceeds: 335ms Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
The Ocap Kernel team needs a new build type in order to begin integrating with the extension. This build should be identical to Flask, except that it will include one additional feature. To accomplish this without maintaining a configuration for a single build twice, this PR introduces a new field
extendsto build types inbuilds.yml. A build type can thereby extend another build type by specifying its name to thebuildType.<name>.extendsproperty.The format of
builds.ymlis otherwise unaffected, with one exception: it used to be possible to specify aenvproperty onfeatures.<name>. Since we didn't do this in practice, this optional field is no longer recognized.As part of this change, we DRY up the loading of
builds.ymlin the old and new build systems. Due to its more extensive validation, the old system's JavaScript implementation is used. The relevant file,development/lib/build-type.jsis now reasonably unit-tested. Passing tests were written both before and after the changes made to that file in this PR. Several improvements to the validation ofbuilds.ymlwas made during the writing of tests, mostly related to describing configuration errors. All types related tobuilds.ymlare now exported frombuild-types.js, and this file is type checked.Why not YAML merge keys?
We forego the use of YAML's "merge keys" feature since it cannot be used to merge lists, and refactoring
builds.ymlto only use dictionaries would require a significant refactor of the original build system. Although this refactor is desirable, we cannot justify it at this time due to the JavaScript source code and low test coverage of the original build system.Manual testing steps
yarn start, observe that it worksyarn webpack, observe that it worksPre-merge author checklist
Pre-merge reviewer checklist