Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix for CJSImportProcessor not using interop helper for aliased default imports #619

Merged
merged 1 commit into from
Jun 6, 2021

Conversation

Rugvip
Copy link
Contributor

@Rugvip Rugvip commented Jun 5, 2021

Hi! Found something that I think is a bug in the handling of aliased default imports.

For example, given the following code:

import { default as myFunc } from './myFunc';

myFunc();

We want this to be transpiled to something like this (simplified):

const myFunc = _interopRequireDefault(require('./myFunc'));

myFunc.default();

But since it's not treated as a default import, we get this kind of thing instead:

const myFunc = require('./myFunc');

myFunc.default();

Which may cause trouble if the imported module is not an ESM module.

@codecov
Copy link

codecov bot commented Jun 5, 2021

Codecov Report

Merging #619 (aa6296e) into main (805cd66) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #619      +/-   ##
==========================================
+ Coverage   82.03%   82.13%   +0.09%     
==========================================
  Files          56       56              
  Lines        5618     5859     +241     
  Branches     1320     1321       +1     
==========================================
+ Hits         4609     4812     +203     
- Misses        725      763      +38     
  Partials      284      284              
Impacted Files Coverage Δ
src/CJSImportProcessor.ts 92.30% <100.00%> (+0.15%) ⬆️
src/util/formatTokens.ts 70.58% <0.00%> (-10.06%) ⬇️
src/parser/util/identifier.ts 90.90% <0.00%> (-4.10%) ⬇️
src/util/shouldElideDefaultExport.ts 77.77% <0.00%> (-3.48%) ⬇️
src/util/getClassInfo.ts 87.21% <0.00%> (-1.93%) ⬇️
src/parser/plugins/jsx/index.ts 91.33% <0.00%> (-1.48%) ⬇️
src/parser/plugins/flow.ts 63.72% <0.00%> (-1.28%) ⬇️
src/identifyShadowedGlobals.ts 87.80% <0.00%> (-1.09%) ⬇️
src/parser/traverser/expression.ts 88.10% <0.00%> (-0.11%) ⬇️
src/Options.ts 100.00% <0.00%> (ø)
... and 20 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 805cd66...aa6296e. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Jun 5, 2021

Benchmark results

Before this PR: 276.5 thousand lines per second
After this PR: 277 thousand lines per second

Measured change: 0.19% faster (1.64% slower to 1.33% faster)
Summary: Likely no significant difference

Copy link
Owner

@alangpierce alangpierce left a comment

Choose a reason for hiding this comment

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

Good catch, and the fix looks great, thank you! I also checked the Babel and TS output and confirmed that both of them have this behavior as well.

@alangpierce alangpierce merged commit 2b50eef into alangpierce:main Jun 6, 2021
@Rugvip Rugvip deleted the rugvip/default-as branch June 7, 2021 12:14
@alangpierce
Copy link
Owner

Just published as 3.18.2

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants