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
feat: treeshake for optional chaining #4797
feat: treeshake for optional chaining #4797
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.
Nice, great work!
Codecov Report
@@ Coverage Diff @@
## master #4797 +/- ##
==========================================
- Coverage 99.02% 99.01% -0.02%
==========================================
Files 217 218 +1
Lines 7780 7792 +12
Branches 2161 2166 +5
==========================================
+ Hits 7704 7715 +11
- Misses 24 25 +1
Partials 52 52
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
||
getLiteralValueAtPath(): LiteralValueOrUnknown { | ||
if (this.getObjectValue() == null) return undefined; | ||
return UnknownValue; |
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.
This one should be easy to cover in a test, I wonder why you current test does not hit this line
private objectValue: LiteralValueOrUnknown | typeof unset = unset; | ||
|
||
deoptimizeCache(): void { | ||
this.objectValue = UnknownValue; |
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.
This one is actually hard to cover in a test and I am fine if it is not covered. To hit this, you have to use a variable further up in the chain that is initialized as undefined
but later gets another value assigned, invalidating the object.getLiteralValueAtPath
in line 46.
The problem is to find a suitable "later". Sometimes it works to do this in a function but as I said, it is tricky.
Updated. Thanks for the guidance! I will leave the coverage |
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.
Great stuff, thanks!
This PR has been released as part of rollup@3.10.0. You can test it via |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
esbuild treeshakes it as well: https://esbuild.egoist.dev/#W1siaW5kZXgudHMiLHsiY29udGVudCI6ImlmICh1bmRlZmluZWQ/LmEpIHtcbiAgY29uc29sZS5sb2coXCJcIik7XG59XG5cbnVuZGVmaW5lZD8uYSgpO1xuIn1dLFsiZXNidWlsZC5jb25maWcuanNvbiIseyJjb250ZW50Ijoie1xuICBcImZvcm1hdFwiOiBcImVzbVwiLFxuICBcImNkblVybFwiOiBcImh0dHBzOi8vY2RuLnNreXBhY2suZGV2XCIsXG4gIFwibWluaWZ5XCI6IHRydWVcbn0ifV0sWyJzdW0udHMiLHsiY29udGVudCI6ImV4cG9ydCBjb25zdCBzdW0gPSAoYTogbnVtYmVyLCBiOiBudW1iZXIpID0+IGEgKyBiIn1dXQ==