-
-
Notifications
You must be signed in to change notification settings - Fork 2k
chore: Bump terser #4791
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
base: main
Are you sure you want to change the base?
chore: Bump terser #4791
Conversation
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Resultscreate10kduration
usedJSHeapSize
filter-listduration
usedJSHeapSize
hydrate1kduration
usedJSHeapSize
many-updatesduration
usedJSHeapSize
replace1kduration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
text-updateduration
usedJSHeapSize
tododuration
usedJSHeapSize
update10th1kduration
usedJSHeapSize
|
Size Change: +13 B (+0.03%) Total Size: 46.6 kB
ℹ️ View Unchanged
|
// @ts-expect-error TODO: Legitimate type mismatch that our use of `NULL` hid | ||
oldVNode ? oldVNode._dom : parentDom.firstChild, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a weird one. Above, we have this:
let oldVNode = isHydrating ? NULL : parentDom._children;
For whatever reason, type inference has failed for NULL
, so oldVNode
was actually any
. As such, this is a legitimate (internal) type mismatch in what diff
expects as a parameter and what we provide. Will fix separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Terser was locked in #4687 as we saw significant perf degradation from bumping the version further. Upon further investigation, it turns out this was due to a lack of hoisting:
In our CJS & ESM builds,
NULL
&UNDEFINED
are not hoisted as constants but inlined throughout the bundle. For reasons that I still don't quite understand, hoisting these two, as newer versions of Terser will do, is a huge perf issue for us, causing double-digit regressions across the board. The byte impact is completely overshadowed by the perf hit, so, I've removed theNULL
&UNDEFINED
constants. We'll go back to usingnull
&undefined
/void 0
throughout the source.Namespaces are a similar story; right now they're inlined, newer versions of Terser will hoist, but this is a very tiny byte reduction (we reference Math only once and XHTML & SVG twice each) with a 1-4% perf degradation in a few tests. Inlining them too seems better.
Edit: The perf regression is much larger w/ JIT enabled, disable it & hoisting is a slight improvement in some, still a regression in others. Not that it matters much ofc, just an interesting tidbit.
Therefore, the only real changes of this PR are as follows:
version
is now hoisted too, instead of inlined into the CJS exports. Harder to fix (or at least, I'm not sure how to) as we don't easily control how the exports are generated.exports.version = '18.3.1'
has becomeexports.version = P
No worries if we want to just stick w/ older Terser, I mostly did this as an exploration as I was curious. There's no definite advantage here (I could go bench UMD builds I suppose though), just refactoring us so new Terser lines up with our old Terser output.