-
Notifications
You must be signed in to change notification settings - Fork 884
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
fix an obnoxious uglify issue #2039
Conversation
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.
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. |
@Feiyang1 Can you add a Changelog entry for the RTDB component? |
Okay. I added |
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. :) |
@schmidt-sebastian @ryanpbrewster |
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. |
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 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. |
Decision: Do not pin |
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.
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?
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.
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. :-/
Is this issue just the root cause for #2035? |
Yes (and for many other customer reports). |
@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
is turned into:
The issue doesn't exist in the latest
uglify-js
anymore, so we upgradeuglify-js
to 3.6.0Fixes: #2035