Please consider rethinking the way how plugins access acorn

[lukastaegert]

Hi acorn team,

first let me as current maintainer of Rollup express my gratitude for this awesome project that we depend heavily on, you are doing really great work!

The issue at hand is something that has been bothering me deeply ever since we updated to acorn 6. Rollup has always had the policy of bundling all its dependencies but with acorn 6, we were forced to make acorn an external dependency. Why so?

How acorn forced its first external dependency upon Rollup

Apparently it is common in the acorn ecosystem for plugins to require('acorn') to gain access to acorn and specific things like token types etc. Now this means that if Rollup were to bundle acorn, an acorn plugin supplied by a Rollup user would use a different acorn instance as Rollup itself. For acorn 5 apparently this was not that big of an issue as it seems a lot hinged on string comparisons but with acorn 6, some of those things now seem to depend on reference comparisons which obviously fails if there is more than one acorn instance involved.

So we grudgingly had to accept that acorn plugins will only work if acorn is external. Now this may be an option for the Node version of Rollup, but for the browser version this either means that one now has to start piling up script tags in the correct order—one for acorn, one for Rollup. This was hard to accept for us as so basically the browser build of Rollup no longer supports acorn plugins.

But there are more problems:

If plugins work depends on how a user manages dependencies

Recently, we got this issue rollup/rollup#2766 from a user who unsuccessfully tried to use the acorn-bigint plugin. Of course he blamed Rollup but after some debugging it turned out the actual issue was that there was more than one node_modules folder involved and the plugin again had a different acorn instance than Rollup.

Now these situations may not be too common but they are hard to track down. And even with a single node_modules folder, things can go awry. I am not sure if you are aware of this but this webpack issue webpack/webpack#8656 hit a lot of users and even though the problem was put to npm, IMO the core problem is that acorn plugins directly import acorn.

This is an important decision for future plugins

So I know you will not be able to solve this easily as basically the solution will mean that all plugins need to be changed. Nevertheless I hope I could communicate the problem to you, and my hope is that some work is undertaken to move acorn to a dependency-injection/inversion-of-control approach, which IMO is always a really good idea for plugin systems (and something observed by the plugin systems of Webpack and Rollup. Rollup even provides a premade acorn parser to Rollup plugins so that they do not need to depend on acorn).

So basically when creating a parser with plugins, it would be good if the plugins gained access to acorn via this process, e.g. as an argument passed to them. Depending on how you do it, this could be a non-breaking change initially, allowing you to migrate core plugins one by one. Then at some point, the unwanted exports could be removed from the acorn package or renamed so that plugins can no longer rely on them.

This is just some food for thought in the hope to influence some future decisions.

[marijnh]

As a maintainer of Rollup, you're surely aware of the benefits of ES6-style imports (direct access to bindings, tree shaking, etc), which are mostly undone when treating modules as values—which would be necessary to do this kind of library-passing. Not sure how much of acorn is shakeable (since the system itself references most of its componsents), but since the community as a whole seems to be moving towards ES6 modules and direct imports, I designed the interface along these lines, and the scheme you describe (passing the acorn module to plugins) seems antithetical to that.

Dependency management is a pain, and I'm not downplaying the issues you run into, but I don't think package interfaces are the place to solve them.

[mysticatea]

In my understanding, the problem is that token types objects possibly come from a different location from the Parser class because the Parser class is passed as a parameter to plugins, but token types are not. However, the Parser class uses many reference comparison around the token types. This is not reliable. It looks reasonable if the Parser provides token types instances the parser actually uses.

[lukastaegert]

Without looking further into it, this sounds like a reasonable and sufficient way of addressing the issues we have.

[marijnh]

I'd say that, in general, if you're accidentally loading multiple versions of a module, that's a problem, and you'd want to address that rather than paper it over, no?

[mysticatea]

I think it's case by case because npm fails dedupe for a variety of reasons. Yes, if the code size is important (e.g., for browsers), then I want to dedupe. But I often don't mind it on CLI tools.

At least, this problem is tough to investigate; an understandable error message would be valuable.

[marijnh]

an understandable error message would be valuable.

I agree with that, but I'm not sure where/how that could be efficiently checked—especially since it's possible for plugins to define new token types.

[lukastaegert]

I agree there are situations where it is an accident that several versions of acorn are in the module graph, such as plugins using actual dependencies instead of peer dependencies. But what about situations where there are two valid consumers users of acorn are in the dependency graph, e.g. a future version of Webpack using an (imaginary) acorn 7 which is used to bundle a Rollup version using acorn 6? Plugins meant for Rollup might wrongly try to use Webpack's acorn dependency, breaking them in the bundled output.

And going back to the original issue we had with Rollup, the only possible solution for bundling acorn again would be to completely hijack npm's module loader, catching all require('acorn') to point to the bundled version. Which is SURE to cause other issues and is not doable for the pure JS interface of Rollup anyway.

There is a myriad of variations on this issue. My suggestion is not necessarily to attach all of acorn to an object but to move to a controlled plugin interface where the current instance of acorn has control over what plugins receive. This would solve all of these issues.

[mysticatea]

I agree with that, but I'm not sure where/how that could be efficiently checked

For example, the Parser class provides verifyTokenType(num) static method and plugins call it with tt.num when a plugin defines the sub class of Parser.... though this is not smart.

[marijnh]

If you're going to have plugins explicitly check, importing TokenType and comparing it to parser.type.constructor should be enough.

[PerspectivesLab]

could this be looked into ? its blocking all future minor versions of webpack while doing :
import( /* options */ 'filename' )

[tony]

For more information on what @PerspectivesLab is mentioning, see webpack/webpack#8656 (around early July 2019) is where we're at now.

There's a dependency graph of stuff getting pulled upstream to webpack that's sort of hard to untangle. Frankly this is the first time I heard of acorn (despite using it as a dependency every day)

[lukastaegert]

Hi, sorry for reviving this old issues but as it turns out, native ES module support in Node has added another unfortunate facet to this story that I want to highlight in hope of someday swaying the stance of acorn maintainers.

At the moment I am working on making it possible to use Rollup as a native ES module import in Node and again, I am hitting a Roadblock here due to acorn's decision that plugins independently import acorn instead of getting the necessary dependencies injected by the caller.

In a nutshell, the problem is the following:

• If the ESM build of Rollup is configured to import the ESM build of acorn, plugins written in CJS cannot be used as they will pull in the CJS build of acorn.
• If the ESM build of Rollup is configured to import the CJS build of acorn, we need to make sure we only use a default import because that is all an ES module can import from a CJS module in Node. This will break if a bundler uses the "module" package field to replace it with the ESM version of acorn as this version does not have a default export. So we will need to make sure the ESM version of Rollup also imports the CJS version of acorn even when bundled, e.g. by using a deep import. This will mean additional overhead for anyone bundling Rollup itself, e.g. StencilJS.

The second issue can be somewhat fixed if the ESM build of acorn also contains a default export. But it should be clear that this argument from #824 (comment)

As a maintainer of Rollup, you're surely aware of the benefits of ES6-style imports (direct access to bindings, tree shaking, etc), which are mostly undone when treating modules as values—which would be necessary to do this kind of library-passing

is rendered completely void by this.

And this is not only Rollup: acorn plugins that want to work in a Node ESM setup will have a hard time importing acorn "the right way" as well.

So to sum it up:

1. If you bundle acorn, it is currently impossible to use non-bundled acorn plugins
2. If you do not bundle, you need to make sure even ESM code always imports the CJS version if you want to use acorn plugins

And this is in addition to all the multiple-acorn-versions in the dependency tree are breaking my foo issues that already exist.

So please, please consider changing your architecture here! The tree-shaking advantages are probably negligible, but the pain this decision is causing us and others every day is very real. Note that we are otherwise really happy with acorn except for this issue 😢

[lukastaegert]

Actually, even forcing Rollup to import the CJS version of acorn will not fix all issues: If a user now decides to bundle Rollup together with acorn and a custom acorn plugin, due to how bundlers work, the custom plugin will now probably and paradoxically pull in the ESM version of acorn, and the whole bundle will be broken again. So the ONLY way Rollup can support Node ESM at the moment is to provide two slightly different ESM builds alongside the CJS build, one for Node ESM and one for rebundling. Does not feel good.

[marijnh]

Have you seen #870 ? Does it (assuming plugins all use it) solve your concern?

[lukastaegert]

Actually it looks perfect! Sorry for not noticing it 🙄 I will try it out and close this issue if there are no blockers. Then Rollup@2, which is bound to be released soon, will be back to bundled acorn and also support Node ESM 🎉 This might also put some pressure on migrating more plugins to support this, but it seems the necessary change is rather simple.

[lukastaegert]

Thanks so much! I cannot tell you how happy this makes me 😉 This was the last missing piece for Rollup@2 which will be released shortly.