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

Upgrading to Acorn 6 #2486

Merged
merged 3 commits into from
Oct 28, 2018
Merged

Upgrading to Acorn 6 #2486

merged 3 commits into from
Oct 28, 2018

Conversation

marijnh
Copy link
Contributor

@marijnh marijnh commented Oct 1, 2018

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

Description

This is to get the ball rolling up upgrading to Acorn 6, a new major version of the parser. There's two contentious parts:

  • The plugin API in Acorn 6 works differently, which means the acornOptions.plugins option to the parse method will work differently after the upgrade. It'll work better, since plugins are now just values and easier to pass around, but code that uses this option in the old way will break.

  • I had to add an ugly kludge to rollup.config.js to make the build go through. If you remove the return statement I added, the build fails with a 'default' is not exported warning, which points at node_modules/acorn/dist/acorn.mjs (which indeed does not export default), as apparently imported by its own rollup-commonjs generated code. I was unable to figure out why this warning came up (the old module also didn't export default), and I'm hoping someone here can help figure that out.

@lukastaegert
Copy link
Member

lukastaegert commented Oct 2, 2018

Hi @marijnh, awesome to get this going. I also had a poke at this yesterday (since I read that dynamic imports are now acorn-6 compatible) so I have some insights into the "default is not exported" error.

This error is caused by rollup-plugin-commonjs and IMO can be fixed there (working on this plugin anyway right now). It is caused by the two acorn plugins we include by default which in turn require acorn. As rollup by default uses the .mjs version of acorn, these plugins are also redirected to use this version.

Now since those plugins are CJS and acorn is ESM, the CJS plugin will add a proxy wrapper around their import of acorn. Since the extension of acorn is .mjs, however, the CJS plugin will not check if the plugin has a default export but instead add a runtime check. This check is written as

import * as acorn from 'acorn';
export default ( acorn && acorn['default'] ) || acorn;

rollup again tries to simplify the namespace property access which fails because there is no default export (I believe the reason for using the computed property was to prevent this but this only worked in earlier versions of rollup).

I think the only safe way of handling this is to do default export detection for all files, not only those with a .js extension. Will look into this a little. At the same time I think I will try to improve the interop code to a more robust

import * as acorn from 'acorn';
export default acorn && 'default' in acorn ? acorn.default : acorn;

but I fear this will not solve the issue.

Update: Maybe it will work if I put the wrapper into a function as this will force Rollup to create a namespace object. A little more code but easy to achieve.

@lukastaegert
Copy link
Member

I did the necessary updates to your branch, fixed the rollup-plugin-commonjs issue, types and tests. Please have a look!

@lukastaegert lukastaegert changed the base branch from master to 1.0 October 11, 2018 04:42
@lukastaegert
Copy link
Member

Since you correctly noted that this will be a breaking change due to the changes in which and how acorn plugins can be provided, I changed the base branch to the 1.0 release.

@lukastaegert lukastaegert changed the title WIP Upgrading to Acorn 6 Upgrading to Acorn 6 Oct 11, 2018
@adrianheine
Copy link
Contributor

This allows to use acorn-export-ns-from, too :)

@guybedford
Copy link
Contributor

@adrianheine probably worth creating a tracking issue for that, as it will likely be buggy :)

@lukastaegert lukastaegert merged commit ba10576 into rollup:1.0 Oct 28, 2018
lukastaegert pushed a commit that referenced this pull request Oct 28, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Oct 28, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Oct 28, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Oct 30, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Nov 17, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Nov 17, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Dec 8, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Dec 11, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Dec 12, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Dec 15, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Dec 20, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Dec 20, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Dec 20, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Dec 23, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Dec 28, 2018
* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests
lukastaegert pushed a commit that referenced this pull request Dec 28, 2018
* Add English docs to repo

* emit watch changes as a plugin hook

* deprecate asset dependencies

* 1.0 API changest status

* watcher interface simplifications

* Add new option errors

* Make sure only valid JS is piped to stdout

* Refine error handling to include dyamic imports

* Fix output sorting

* 1.0: deprecations (#2409)

* emit watch changes as a plugin hook

* fix path require

* deprecate asset dependencies

* renderChunk hook to replace transformChunk

* renderChunk to follow all transformBundle tests, type definition

* 1.0 deprecations

* legacy deprecations

* new deprecation tests

* type fix, output options deprecations

* type fixes

* deprecation deprecations

* Only print file ids in plugin warnings and errors if present

* Upgrading to Acorn 6 (#2486)

* WIP Upgrading to Acorn 6

* Fix rollup-plugin-commonjs issue, remaining type issues and tests

* Sort options list in docs (no content changes)

* Make sure --input option supports named entries as well as default CLI options

* Make preferConst an output option

* Small type fixes

* Externalise acorn

* Allow the `d` alias for the `--dir` option

* Further refine types and make sure returning false from resolveId marks
imports as external

* Thoroughly review and update docs
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