Navigation Menu

Skip to content
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 additional let/var init bugs #4177

Merged
merged 4 commits into from Jul 15, 2021
Merged

Fix additional let/var init bugs #4177

merged 4 commits into from Jul 15, 2021

Conversation

kzc
Copy link
Contributor

@kzc kzc commented Jul 12, 2021

Fix additional variable state change bugs including blockless if statement var declarations and let/var use before declaration within same function that do not cross function scopes.

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

Fixes several outstanding var/let init issues mentioned in #4148 without any code pessimization other than for previously incorrect rollup output, and with minimal overhead.

including blockless if statement `var` declarations
and let/var use before declaration within same function
@codecov
Copy link

codecov bot commented Jul 12, 2021

Codecov Report

Merging #4177 (ff8a078) into master (004f800) will increase coverage by 7.06%.
The diff coverage is 99.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4177      +/-   ##
==========================================
+ Coverage   91.27%   98.33%   +7.06%     
==========================================
  Files         170      202      +32     
  Lines        5923     7227    +1304     
  Branches     1794     2114     +320     
==========================================
+ Hits         5406     7107    +1701     
+ Misses        311       58     -253     
+ Partials      206       62     -144     
Impacted Files Coverage Δ
browser/path.ts 76.92% <ø> (ø)
cli/run/timings.ts 0.00% <0.00%> (ø)
src/ast/CallOptions.ts 100.00% <ø> (ø)
src/ast/ExecutionContext.ts 100.00% <ø> (ø)
src/ast/nodes/ExpressionStatement.ts 87.50% <ø> (ø)
src/ast/nodes/NewExpression.ts 100.00% <ø> (ø)
src/ast/nodes/NodeType.ts 100.00% <ø> (ø)
src/ast/nodes/ObjectExpression.ts 100.00% <ø> (+9.58%) ⬆️
src/ast/nodes/ObjectPattern.ts 88.23% <ø> (+1.56%) ⬆️
src/ast/nodes/Program.ts 100.00% <ø> (ø)
... and 317 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccdf124...ff8a078. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Jul 14, 2021

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install kzc/rollup#additional-variable-init-fixes

or load it into the REPL:
https://rollupjs.org/repl/?pr=4177

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

I took the liberty of replacing the removed example in the docs with one that is still failing, otherwise we can merge this for now.

@kzc
Copy link
Contributor Author

kzc commented Jul 14, 2021

Works for me.

docs/999-big-list-of-options.md Outdated Show resolved Hide resolved
@lukastaegert lukastaegert merged commit 7c014fb into rollup:master Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants