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
workaround for various object literal mutation bugs #3266
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3266 +/- ##
==========================================
- Coverage 94.19% 92.76% -1.43%
==========================================
Files 170 170
Lines 7992 5943 -2049
Branches 2870 1796 -1074
==========================================
- Hits 7528 5513 -2015
+ Misses 293 225 -68
- Partials 171 205 +34
Continue to review full report at Codecov.
|
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.
Thanks a lot for the P!. Even though by now I have a fix for the root cause of #3264 via #3267, I would be interested in merging the other changes related to unsafe this
modifications. Obviously, this has been lying around for very long and I have not gotten around to providing the necessary fix myself. If you remove the other change, this can be merged.
One other thing: This does NOT fix the third case mentioned in #2345, so it would be nice to remove #2345 from the list of fixed issues so that it is still tracked.
This PR does indeed fix the third test case in #2345 as seen here: input: https://github.com/rollup/rollup/pull/3266/files#diff-e327407bfb6a2951b6ffb6e99b6b6dbdR31-R37 output: https://github.com/rollup/rollup/pull/3266/files#diff-f9e32e571549a0ee3077bed0c7b2d828R21-R27 It's a slight variation of the test case in question, but it also runs correctly on the original test case:
with this PR:
without this PR:
Feel free to modify this PR or ignore. I just wanted to demonstrate a workaround. The |
My bad, I only copied over the CallExpression change but did not check that it fixes the last case only together with the other change. I will modify this and merge it in, thanks! |
…`this` by default
@@ -66,6 +67,9 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt | |||
this | |||
); | |||
} | |||
if (this.callee instanceof MemberExpression && !this.callee.variable) { |
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.
Funny - I had this initially but dismissed it due the higher number of test regressions. But it's the safer route.
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.
Well, if we gonna fix it, we might just as well fix it all the way down. I kept the tests because I REALLY hope to get in a proper solution soon.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
#2345 #2473 #3027 #3093 #3192 #3264
Description
Workaround for various object literal mutation bugs.
Some necessary minor regressions related to previously unsafe object literal optimizations.
Fixes #2345 #2473 #3027 #3093 #3192 #3264