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

chore: bump acorn to 8.0.1 #11550

Closed
wants to merge 1 commit into from
Closed

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Sep 29, 2020

closes #11186
closes #10227

This PR aims to bump acorn so ?., ??, 4_2, x ??= y should not throw syntax error: It is part of ES2021.

This PR does not backport evaluations support in #11221. I think it is good enough for webpack@4. To further optimize expression related to these new syntax features, please upgrade to webpack@5. But the bottom line here is that webpack@4 should not throw for ES2021 syntax, especially when webpack 5 is not released yet.

What kind of change does this PR introduce?

bugfix

Did you add tests for your changes?

No. The acorn tests itself should be sufficient.

Does this PR introduce a breaking change?

Yes. See https://github.com/acornjs/acorn/blob/master/acorn/CHANGELOG.md#700-2019-08-13 Though acorn v7 to v8 is not breaking webpack 4.

What needs to be documented once your changes are merged?

Nothing.

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@alexander-akait
Copy link
Member

@JLHwung I think we don't need this because webpack@5 ETA is 2020-10-10, so better to update acorn for webpack@5 to 8 version

@JLHwung
Copy link
Contributor Author

JLHwung commented Sep 29, 2020

@evilebottnawi I think it will be great to bump acorn on webpack@4 because after webpack@5 is released, it will take a while for communities to catch up. During this period, people may use latest syntax and the error thrown from webpack@4 is thus confusing:

Module parse failed: Unexpected token (2:10)
File was processed with these loaders:
 * ./node_modules/ts-loader/index.js
You may need an additional loader to handle the result of these loaders.
| var a = { b: 2 };
> var c = a?.b;

This is not some customized JS syntax that must be handled by loader. In this way I consider bumping acorn as a bugfix to webpack@4.

@sokra
Copy link
Member

sokra commented Sep 29, 2020

It's a breaking change and we can't do it for webpack 4.

@JLHwung
Copy link
Contributor Author

JLHwung commented Sep 29, 2020

@sokra I see. If it is a breaking change because

Changes the node format for dynamic imports to use the ImportExpression node type, as defined in ESTree.

It will be great to make it clear at the webpack@4 docs, that webpack@4 supports only ES2019 features. Any ES2020 features is not supported and should be handled by additional loader.

Should we discourage practise like

"resolutions": {
    "acorn": "8.0.1"
},

?

@webpack-bot
Copy link
Contributor

@JLHwung The most important CI builds failed. This way your PR can't be merged.

Please take a look at the CI results from travis (error) and appveyor (failure) and fix these issues.

@JLHwung JLHwung closed this Sep 29, 2020
@JLHwung JLHwung mentioned this pull request Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants