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

remove acorn-bigint and acorn-dynamic-import devDenpendencies #3058

Merged
merged 9 commits into from Aug 14, 2019
Merged

remove acorn-bigint and acorn-dynamic-import devDenpendencies #3058

merged 9 commits into from Aug 14, 2019

Conversation

LongTengDao
Copy link
Contributor

@LongTengDao LongTengDao commented Aug 13, 2019

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

acorn built with bigint and dynamic import internal since 6.2.0 (while rollup depending on acorn ^6.2.1): https://github.com/acornjs/acorn/blob/master/acorn/CHANGELOG.md

(Note: don't update dependency acorn version to 7.0.0 for the moment, until most plugins compatible with that (at least waiting for acorn offcial stage3 plugins), and notice that import() has updated to ImportExpression in 7.0.0, which maybe need to update in rollup source.)

@LongTengDao
Copy link
Contributor Author

LongTengDao commented Aug 13, 2019

@lukastaegert Could you figure out why the checks failed? If I missed something intricate, just treat this PR as an issue for refactor notice.

@lukastaegert
Copy link
Member

lukastaegert commented Aug 13, 2019

The PR changes how the acorn import manifests in the ESM build. There is a special plugin that scans for how the import is written to rewrite it so that the ESM build can be reused properly by ESM aware bundlers. What needs to be done is basically to replace the generated default import (which comes from a poor CommonJS translation) with a namespace import. To that end, the two constants expectedAcornImport and newAcornImport need to be changed so that they line up with what can be found in the generated bundle.

@LongTengDao
Copy link
Contributor Author

@lukastaegert Thanks for your explanation! I think I can't handle it. I will just leave this as an issue, waiting for someone else to do that.

@lukastaegert
Copy link
Member

I fixed the import plugin but it appears dynamic imports are no longer recognized with these changes.

@lukastaegert
Copy link
Member

Ok, seems they were lying about the minimum ECMA version, it needs to be 2020 (or 11) to work, not 10.

@codecov
Copy link

codecov bot commented Aug 13, 2019

Codecov Report

Merging #3058 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3058      +/-   ##
==========================================
- Coverage   88.73%   88.73%   -0.01%     
==========================================
  Files         165      165              
  Lines        5735     5733       -2     
  Branches     1748     1748              
==========================================
- Hits         5089     5087       -2     
  Misses        388      388              
  Partials      258      258
Impacted Files Coverage Δ
src/Module.ts 94.19% <ø> (ø) ⬆️
src/Graph.ts 92.98% <ø> (-0.09%) ⬇️

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 d18c12b...8e43b69. Read the comment docs.

@LongTengDao
Copy link
Contributor Author

LongTengDao commented Aug 14, 2019

Oh, 11 is right. I will recorrect the changelog / doc / .d.ts of acorn.

@lukastaegert lukastaegert merged commit 2593b04 into rollup:master Aug 14, 2019
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.

None yet

2 participants