fix an obnoxious uglify issue

[Feiyang1]

@ryanpbrewster reported an issue where uglify-js uglify this function incorrectly. As a result, the uglified code does wrong thing on multi-byte utf8 characters.
This function is being used in RTDB, so some user data in RTDB might have been corrupted.

As an illustration

arr[idx++] = 0;
arr[idx++] = 1;

is turned into:

arr[idx++] = (arr[idx++] = 1, 0)

The issue doesn't exist in the latest uglify-js anymore, so we upgrade uglify-js to 3.6.0

Fixes: #2035

[ryanpbrewster]

I can't think of any way to prevent regressions here. Can we maybe add this to a package.json somewhere?

[schmidt-sebastian]

I can't think of any way to prevent regressions here. Can we maybe add this to a package.json somewhere?

Isn't this a regression in UglifyJS? They could break us again with a 3.6.1 release if they don't have tests for this.

[schmidt-sebastian]

@Feiyang1 Can you add a Changelog entry for the RTDB component?

[Feiyang1]

Okay. I added [email protected] as a direct dependency in Firebase and excluded it from auto dep update, so we should always get 3.6.0 going forward.

[mikelehen]

Is the plan to pin indefinitely? That seems non-ideal. Presumably uglify will make improvements in the future that improve bundle size, etc. and we won't get them. 😢

Sebastian thinks this is the fix on their end: https://github.com/mishoo/UglifyJS2/pull/3430/files and they added a regression test.

It would not be unreasonable for us to also add a regression test, assuming it's not too convoluted to trigger the bug... So we could do that too.

Not psyched about pinning forever. :)

[Feiyang1]

@schmidt-sebastian @ryanpbrewster uglify-js did add a test for this use case - mishoo/UglifyJS@008c236
but I still think it's a good idea to fix uglify-js version as there is no way for us to verify our minified bundles.

[mikelehen]

FWIW- Firestore has (had?) the ability to run (most of) our integration tests against minified builds here: https://github.com/firebase/firebase-js-sdk/tree/master/integration/firestore But unfortunately that somehow got disabled and so they've rotted now.

Ideally we'd invest in testing our minified builds at some point. There are lots of ways to break the minified build through our own code changes in addition to UglifyJS changing out from under us. And I suspect us breaking ourselves would be more common (it's happened several times, whereas this is the first/only time UglifyJS has broken us).

So I still think pinning is an over-reaction. UglifyJS has been very stable in the past and they (seem to) do a good job of adding regression tests.

[ryanpbrewster]

My main goal for this PR is to ensure that we don't fall back behind v3.4.10 (which fixed this particular issue). IMO it is safer for us to do this directly in package.json than in yarn.lock.

Agreed that we should run integration tests against the minified builds.

No particular opinion on pinning. It does seem like the number of bugs in uglify-js is going down rather than up, and I appreciate that each fix has a regression test accompanying it. I think there's not a ton of danger in ratcheting forward.

[Feiyang1]

Decision: Do not pin uglify-js version as it is fairly stable and added test to prevent regression, so we are not too concerned about it breaking us again and we want to get minification improvements with new versions.

[mikelehen]

Sorry, on second look... why are we only modifying yarn.lock and not a package.json file? Doesn't that put us at risk of reverting this in the future?

[mikelehen]

Nevermind. I guess this change is compatible with our current package.json and wouldn't revert (unless rollup-plugin-uglify explicitly reverts)... So I think this is okay.

But it does seem like we have a problem in that our transitive dependencies don't auto-update because we don't auto-recreate our yarn.lock. :-/

[puf]

Is this issue just the root cause for #2035?

[schmidt-sebastian]

Is this issue just the root cause for #2035?

Yes (and for many other customer reports).