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 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 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

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 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
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants