fix(bundle-size-tests): Update version numbers in bundles #16233
Conversation
github-actions
bot
added
area: examples
Updated the PR description. |
Co-authored-by: Matt Rakow <ChumpChief@users.noreply.github.com>
| @@ -30,6 +49,17 @@ module.exports = { | |||
| use: "ts-loader", | |||
| exclude: /node_modules/, | |||
| }, | |||
| { | |||
| test: /\.js$/, | |||
There was a problem hiding this comment.
Disclaimer: I'm not the most well versed in webpack.
We can't be more specific and target packageVersion.js here? Is this already working on post-bundling files?
There was a problem hiding this comment.
yeah, that should be doable provided this is the only place the version string shows up (I think that's true). Loaders are chained and invoked bottom-to-top as middleware during bundling.
This webpack config points at a bunch of entrypoint files which transitively reference code that's already been compiled in dependent packages. So this config should work fine. But if a package that had a packageVersion.ts was additionally an application (we have an example package like this--webflow I think), we'd want to put this rule before ts-loader in the list, since usually webpack configs will target source files for the package they live in.
There was a problem hiding this comment.
I think it's potentially reasonable to leave it as .js fwiw. Comes down to which we think is more likely:
- We change something about package version generation such that we introduce dynamically generated package versions into non-
packageVersion.jsfiles - We introduce code in our packages which is semantically not a package version but happens to align exactly with one
I'd lean toward the latter being more of a danger, as once we get off the 2.0.0-internal versioning scheme, there are some other places (usually with protocol/persistent versioning concerns) that people might happen to choose semver-like constructs that have overlap with something we're replacing. It's still pretty minor though, since when we're replacing a version, the string we're searching for has something like a build hash on it (and not just '1.0.0' -> '1.0.0-internal' or something likely to collide).
I do like that putting the test condition as packageVersion.js makes this much more self-documenting though.
There was a problem hiding this comment.
I'd lean toward the latter being more of a danger, as once we get off the 2.0.0-internal versioning scheme, there are some other places (usually with protocol/persistent versioning concerns) that people might happen to choose semver-like constructs that have overlap with something we're replacing. It's still pretty minor though, since when we're replacing a version, the string we're searching for has something like a build hash on it (and not just '1.0.0' -> '1.0.0-internal' or something likely to collide).
I agree with your reasoning. Another mitigating factor here is that this webpack output is not used other than to test bundle sizes. So if we do manage to get the versions wrong it should only lead to confusing +/-1-3 byte differences in the bundle size tests. 😄
So this config should work fine. But if a package that had a
packageVersion.tswas additionally an application (we have an example package like this--webflow I think), we'd want to put this rule beforets-loaderin the list, since usually webpack configs will target source files for the package they live in.
It sounds like it's best to both narrow the test condition to packageVersion.js, and also move it to be before ts-loader (and thus run after it). I made those changes in the latest iteration. Let me know if I misunderstood.
Loaders are chained and invoked bottom-to-top as middleware during bundling.
The docs seemed ambiguous to me at first. The example they give has a single rule that invokes multiple loaders, while this config declares multiple rules with a loader defined. But then I read this: https://webpack.js.org/configuration/module/#ruleloader
Rule.loaderis a shortcut toRule.use: [ { loader } ].
So I guess the example is equivalent to ours.
| @@ -30,6 +49,17 @@ module.exports = { | |||
| use: "ts-loader", | |||
| exclude: /node_modules/, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
nit: dynamically including this loader iff verString !== newVersion is a little more efficient for the webpack build. Probably not very noticeable though.
There was a problem hiding this comment.
even less of a big deal if you take Alejandro's suggestion above of adjusting the test condition.
|
I'm going to go ahead and merge because I think I took the feedback correctly, and if I messed up then the worst case is bundle-size tools are broken (which they sort of are right now 😄) |
LGTM |
can't wait to see 0 bundle diffs where I expect them
Description
We update our version numbers in CI when building PRs. These version numbers make it into our compiled code via the packageVersion.ts file that exists in some projects (see the
build:genvernpm task), so if the version number in the CI pipeline is a different number of bytes than the "true" version number, our bundle size tests calculate bundle size differences of 1-3 bytes for every PR.Changes
I used the version-tools package to parse the build numbers and convert them back into the correct version. I used string-replace-loader to do the replacements.
Related to AB#4457