Skip to content

Unmangle a switch variable for Closure #7136

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 21, 2023
Merged

Unmangle a switch variable for Closure #7136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/firebase/compat/rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ import { emitModulePackageFile } from '../../../scripts/build/rollup_emit_module

const external = Object.keys(pkg.dependencies || {});
const uglifyOptions = {
mangle: true,
mangle: {
// Hack for a bug in Closure regarding switch block scope
reserved: ['__PRIVATE_lastReasonableEscapeIndex']
},
webkit: true // Necessary to avoid https://bugs.webkit.org/show_bug.cgi?id=223533
};

Expand Down
4 changes: 3 additions & 1 deletion packages/firestore/rollup.shared.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

firebase-js-sdk/packages/firestore/src/local/encoded_resource_path.ts

Line 146 in 5e142ee

switch (next) {
causes lastReasonableEscapeIndex 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the fantastic explanation!

// due to a Closure bug.
// See issue: https://github.com/firebase/firebase-js-sdk/issues/5384
reserved: ['_getProvider'],
reserved: ['_getProvider', '__PRIVATE_lastReasonableEscapeIndex'],
properties: {
regex: /^__PRIVATE_/,
// All JS Keywords are reserved. Although this should be taken cared of by
Expand Down