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

Acorn 7.3.0 upgrade #3628

Merged
merged 9 commits into from Jun 12, 2020
Merged

Acorn 7.3.0 upgrade #3628

merged 9 commits into from Jun 12, 2020

Conversation

guybedford
Copy link
Contributor

Acorn just released 7.3.0 with optional chaining support so this PR switches back from the fork we were using to provide this.

I did check that the final ESTree changes don't affect any of the internal code here and as far as I can tell there aren't any issues with this currently, but let me know if I've missed anything.

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

@guybedford
Copy link
Contributor Author

Because the build is performed based on the previous build I've left in the fork, perhaps a way to do this will be to remove the fork in a subsequent PR once this is merged?

@rollup-bot
Copy link
Collaborator

rollup-bot commented Jun 11, 2020

Thank you for your contribution! ❤️

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

npm install rollup/rollup#acorn-upgrade

or load it into the REPL:
https://rollupjs.org/repl/?circleci=11868

@codecov
Copy link

codecov bot commented Jun 11, 2020

Codecov Report

Merging #3628 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3628   +/-   ##
=======================================
  Coverage   96.54%   96.54%           
=======================================
  Files         182      182           
  Lines        6243     6243           
  Branches     1832     1832           
=======================================
  Hits         6027     6027           
  Misses        107      107           
  Partials      109      109           
Impacted Files Coverage Δ
src/Graph.ts 100.00% <ø> (ø)
src/Module.ts 98.90% <ø> (ø)
src/ModuleLoader.ts 100.00% <ø> (ø)
src/ast/nodes/NodeType.ts 100.00% <ø> (ø)
src/ast/nodes/index.ts 100.00% <ø> (ø)
src/utils/options/normalizeInputOptions.ts 98.05% <ø> (ø)
src/utils/pureComments.ts 100.00% <ø> (ø)

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 ad24b6a...c2b2038. Read the comment docs.

@chicoxyzzy
Copy link

chicoxyzzy commented Jun 11, 2020

7.3.1 was released a bit later today

@guybedford
Copy link
Contributor Author

I've also added a commit here to upgrade the plugins, as there was a fix in the private class elements package to work with optional chaining here - acornjs/acorn-private-class-elements@c9857bb.

I also removed the import meta plugin since that is in Acorn core since acornjs/acorn#943.

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.

Thanks!

Because the build is performed based on the previous build I've left in the fork

It is not, there was an alias in the Rollup config which replaced acorn by the fork. Which is why your branch was still running the fork. Fixing this revealed that we also needed the new ChainExpression to prevent Rollup from deoptimizing entirely, which I added.

I also removed the import meta plugin since that is in Acorn core

👍

@lukastaegert lukastaegert merged commit 872e9e2 into master Jun 12, 2020
@lukastaegert lukastaegert deleted the acorn-upgrade branch June 12, 2020 19:48
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

4 participants