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 regression causing incomplete nullish coalesce in some cases #610

Merged
merged 1 commit into from
Apr 12, 2021

Conversation

alangpierce
Copy link
Owner

Fixes #609

The original implementation of the Jest transform (#540) needed to remove
regions of code, but ran into issues with the optional chaining and nullish
coalescing transforms, since those transforms would still emit function calls
around tokens even if they were removed. The implementation fixed those issues
by disabling the optional chaining and nullish coalescing code emit in
removeToken and removeInitialToken. Unfortunately, this broke other cases,
like a nullish coalescing call immediately followed by a type token. The nullish
coalescing implementation expected appendTokenSuffix to be called on the last
token even though it was a type token.

The changes to TokenProcessor actually became unnecessary as of #608 since we
no longer are deleting a region of code, so I reverted the two methods back to
their original implementation, which fixes the issue.

Fixes #609

The original implementation of the Jest transform (#540) needed to remove
regions of code, but ran into issues with the optional chaining and nullish
coalescing transforms, since those transforms would still emit function calls
around tokens even if they were removed. The implementation fixed those issues
by disabling the optional chaining and nullish coalescing code emit in
`removeToken` and `removeInitialToken`. Unfortunately, this broke other cases,
like a nullish coalescing call immediately followed by a type token. The nullish
coalescing implementation expected `appendTokenSuffix` to be called on the last
token even though it was a type token.

The changes to `TokenProcessor` actually became unnecessary as of #608 since we
no longer are deleting a region of code, so I reverted the two methods back to
their original implementation, which fixes the issue.
@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #610 (a4dd855) into main (de00b66) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #610      +/-   ##
==========================================
- Coverage   82.04%   82.03%   -0.01%     
==========================================
  Files          56       56              
  Lines        5620     5618       -2     
  Branches     1320     1320              
==========================================
- Hits         4611     4609       -2     
  Misses        725      725              
  Partials      284      284              
Impacted Files Coverage Δ
src/TokenProcessor.ts 93.63% <100.00%> (-0.12%) ⬇️

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 de00b66...a4dd855. Read the comment docs.

@github-actions
Copy link

Benchmark results

Before this PR: 265.7 thousand lines per second
After this PR: 262.2 thousand lines per second

Measured change: 1.33% slower (2.73% slower to 3.58% faster)
Summary: Likely no significant difference

@alangpierce alangpierce merged commit beabcc5 into main Apr 12, 2021
@alangpierce alangpierce deleted the fix-incomplete-nullish-coalesce branch April 12, 2021 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3.18.0 regression: Invalid code for nullish coalescing with type assertion
1 participant