Unmangle a switch variable for Closure #7136
Conversation
|
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected ProductsNo changes between base commit (b516abd) and merge commit (3d16603).Test Logs |
| @@ -150,8 +150,10 @@ const manglePrivatePropertiesOptions = { | |||
| // This can be removed if the problem in the downstream library is fixed | |||
| // or if terser's mangler provides an option to avoid mangling everything | |||
| // that isn't a property. | |||
| // `lastReasonableEscapeIndex` was causing problems in a switch statement | |||
There was a problem hiding this comment.
Do you have a link to a buganizer ticket or the "closure bug" for additional context? On its own it's unclear what bug this fixes. Also, why is this the only property that happens to be affected by this "bug"?
There was a problem hiding this comment.
Bug: b/124161650
The issue seems to be that if you declare a variable outside the switch block, then reference it in the switch condition (the parens), and then declare a new variable with the same name inside the switch block (in one of the cases), Closure thinks that the variable inside the switch condition is referencing the variable in the case, which hasn't been declared yet, instead of realizing it's the variable declared before the switch block.
The mangling around this bit of code:
causeslastReasonableEscapeIndex to have the same mangled single-letter name as currentPiece (and the entire if block gets moved into the switch condition as part of tree-shaking/minifiication) which triggers this Closure problem. If we prevent lastReasonableEscapeIndex from being mangled, it can't have a doppelganger variable inside the switch block.
This is a very hacky fix but the only robust alternative I can think of is to generate an additional un-mangled version of Firestore exclusively to be used by the CDN build script (the prod version of @firebase/firestore will continue to be mangled, for size savings). This way it won't be mangled twice. Unfortunately that's one more Firestore build which will make build time longer.
Without the change, this is the code that is generated for packages/firestore/dist/index.esm2017.js:
const n = e - 2, s = [];
let i = "";
for (let r = 0; r < e; ) {
// The last two characters of a valid encoded path must be a separator, so
// there must be an end to this segment.
const e = t.indexOf("�", r);
(e < 0 || e > n) && $();
switch (t.charAt(e + 1)) {
case "�":
const n = t.substring(r, e);
where n is the problem variable. With this fix, the first n is replaced with __PRIVATE_lastReasonableEscapeIndex (the _PRIVATE gets appended by another Firestore build task which I'm not sure if it's worth trying to work around right now) and the problem goes away.
There was a problem hiding this comment.
Thanks for the fantastic explanation!
There is a bug where Closure has trouble handling scope in a switch statement. The mangler reuses a name in the switch statement body that was previously declared outside the switch scope and used inside the switch condition. If we don't mangle the outer variable, there can't be a name collision.