acornjs / acorn-jsx

Alternative, faster React.js JSX parser
MIT License
648 stars 72 forks source link

Allowing acorn ^5 or ^6 as peer dependency breaks any module that relies on acorn 6 if 7 is present #103

Closed iansan5653 closed 4 years ago

iansan5653 commented 4 years ago

Sorry for the confusing title - if anyone can improve it please do.

Problem

This module claims to support both acorn^6.0.0 and acorn^7.0.0:

https://github.com/acornjs/acorn-jsx/blob/e57398c73a0609692ca83214943046dbe7871eea/package.json#L22

This may be correct in this particular module, but by allowing either one as a peer dependency, Node automatically pulls in the most recent one that is available. So if this plugin is being used in a project that still doesn't support acorn^7 but acorn^7 is included somewhere else in the dependency tree, the project will break because the versions conflict.

This is probably best explained with an example:

Example

I've uploaded a basic module at https://github.com/iansan5653/simple-acorn-jsx-example/ that is a minimal example of a module that uses this extension and still runs acorn^6. If you download this and run npm i and node index, the module will run just fine. As expected, npm installs the most recent version of acorn-jsx and acorn^6.3.0, and because acorn-jsx does support acorn^6, this works great.

However, if I require my example module in another project that uses acorn^7, this is where problems occur. Here's an example of such a project:

package.json:

  "dependencies": {
    "acorn": "^7.0.0",
    "simple-acorn-jsx-example": "github:iansan5653/simple-acorn-jsx-example"
  }
}

index.js:

const result = require("simple-acorn-jsx-example")
console.log(result);

Running node index here results in an error. This happens because acorn-jsx finds acorn^7 in its peers and pulls that in instead of using the version of acorn found inside simple-acorn-jsx-example.

Impact

A real-world impact of this is that any project which relies on Buble breaks if that project (or any of its dependencies) relies on acorn^7. By extension, any project that uses styleguideist breaks under the same circumstances, because styleguideist relies on Buble.

Reference: styleguidist/react-styleguidist#1321.

Solution

I'm not sure exactly what solution works best here. Ideally acorn-jsx should check to see which version of acorn the requiring package is using and then adjust accordingly, but that may not be feasible.

Maybe there should be two releases of this plugin - one that is compatible with acorn^4 and one with acorn^5. Maybe these could be two major versions of the same plugin? I'm not really sure.

Maybe there's a much more elegant solution I don't know about.

RReverser commented 4 years ago

Hmm, you're saying that acorn-jsx "pulls in" a different version of acorn, but peerDependencies are not installed automatically by Node.js.

You need to add acorn^6 (or any other desired version) explicitly alongside acorn-jsx - it's not very clear from example above if that's what you're doing? (UPD: looks like you're in that Git repo)

RReverser commented 4 years ago

Actually, I don't see a problem with your example. When I replicate same package.json and index.js, I do see an error, but it's coming from the acorn^6 inside simple-acorn-jsx-example dependencies, and not from the top-level one.

iansan5653 commented 4 years ago

Hmm maybe I'm wrong about the exact cause. I made the demo last night and it did what I expected so I figured I was right.

That said I'm not sure what else could be the cause. simple-acorn-jsx-example runs on its own just fine if it's not being used as a dependency. And it installs its own acorn^6 in its own node_modules if the requiring project installs acorn^7, as you can see in my example. Something is definitely conflicting and I don't think the error still happens if simple-acorn-jsx-example is modified to not depend on acorn-jsx (can't try it at the moment as I'm on mobile).

RReverser commented 4 years ago

And it installs its own acorn^6 in its own node_modules if the requiring project installs acorn^7, as you can see in my example.

That's the expected behaviour, given that simple-acorn-jsx-example depends on acorn^6, no?

RReverser commented 4 years ago

cc @mysticatea could this be similar to the issue you had and described earlier in https://github.com/acornjs/acorn/pull/870 and https://github.com/acornjs/acorn-jsx/pull/102?

mysticatea commented 4 years ago

Yes, this looks the problem that acornjs/acorn#870 and #102 have solved. If you use acorn 7.1.0 or newer, this kind of broken will disappear.

If we want to solve this on acorn 6 as well, we need to backport acornjs/acorn#870.

iansan5653 commented 4 years ago

Backporting to 6 sounds like the ideal solution if that's feasible.

huozhi commented 4 years ago

encounter similar issue here:

having both rollup and buble as dependencies. using rollup@1.25.2 > (deps on) > acorn@7.1.0 using buble@0.19.8, it requires acorn@6, which let npm install its own acorn@6 under buble folder.

then acorn-jsx parsing will break.

iansan5653 commented 4 years ago

Looks like this is resolved by acornjs/acorn#887 🎉