New: Provide ESM package for Espree

[mreinstein]

Summary

Provide an ecmascript module for espree.

Related Issues

eslint/js#457

[mreinstein]

@nzakas I don't believe this is a breaking change, assuming the proposed "dual package" approach is taken.

[nzakas]

eslint/js#457 is already marked as breaking. This is a big enough change that we need to do it in a major version.

[mdjermanovic]

I don't believe this is a breaking change, assuming the proposed "dual package" approach is taken.

Currently with espree v7, if someone does import("espree"), they'll get { default: { parse } }.

If we choose named exports, as in eslint/js#458, they will be getting { parse }.

For example, this gives different results with acorn 7 and acorn 8:

import * as acorn from "acorn";

console.log(Object.keys(acorn));

If we export default { parse }, some builds could be broken, as we experienced with esquery (eslint/archive-website#709 and eslint/archive-website#707).

[mdjermanovic]

There is a script to create UMD, but are we publishing this bundle or using it to run any tests? If neither of those, maybe we should just drop it.

[nzakas]

I can’t think of a good reason to generate UMD. I’m in favor of dropping it.

[nzakas]

I think what you're referring to here is the browserify step we have in the eslint repo? We use that just to create a browser-safe version of eslint for our demo and isn't an officially supported version of Espree. (You can't reference a UMD version from the Espree package.)

[mdjermanovic]

I believe there will be no difference for Node apps/libs that require("espree"), but there are some changes that might break someone else:

• We are changing the import API: certainly when a module that imports espree is run in Node (they'll have to switch from import espree from "espree" to import * as espree from "espree") and maybe when the imports are interpreted by some tools (e.g., bundlers) depending on how the tool works.
• Adding exports to package.json restricts modules that can be imported/required from outside.

[nzakas]

I don't think we'd need to use import * as espree from "espree" based on the latest Node.js compat.

The second point is valid. Just adding exports to package.json is a breaking change.

[ljharb]

Adding "exports" is always a breaking change, unless you explicitly enumerate every current possible requireable entrypoint (npx ls-exports path . if you want to compare). I suggest doing it only as a semver-major.

[mdjermanovic]

Can we specify some details in the RFC document:

• Espree ESM API.
• We're not bundling anything for ESM, just exposing one module as-is from the source code, which then imports other modules and dependencies?
• dist/espree.cjs is a bundle with all espree's own code, but without dependencies?
• Testing strategy
  • Should we run tests on dist/espree.cjs and/or espree.js, or something else?
  • Unit tests for internal modules. I don't think we have many of those, maybe we could just drop them if they're already covered by the top-level espree tests.

[mreinstein]

@mdjermanovic

Can we specify some details in the RFC document:
Espree ESM API.

I'm not sure what the ask is here. In my experimental PR, I did not modify the API of espree; just replaces require with import. Can you clarify what you're referring to?

We're not bundling anything for ESM, just exposing one module as-is from the source code

👍 Ideally the source is already in the format we want and we're accommodating cjs/umd with some build process. Whenever we decide to umd/cjs aren't needed, we can just drop the build step and we're good.

dist/espree.cjs is a bundle with all espree's own code, but without dependencies?

👍 Otherwise we end up bundling in external dependencies like acorn.

In my proof-of-concept PR, I accomplished this via rollup's external config field: https://github.com/eslint/espree/pull/458/files?file-filters%5B%5D=.js#diff-6814bf77564b4f1c92f5861e184e28fe217c080a047fefa8b73a728f755ec45cR23

Testing strategy

Ideally we would add unit tests for importing both cjs and the esm versions of the module. I don't think it would be necessary to duplicate the existing tests for both cjs and esm, just the interface in both cases.

[mdjermanovic]

Can we specify some details in the RFC document:
Espree ESM API.

I'm not sure what the ask is here. In my experimental PR, I did not modify the API of espree; just replaces require with import. Can you clarify what you're referring to?

It would be good to just clarify how the espree ES module can be used:

import * as espree from "espree"; espree.parse(...); or import { parse } from "espree"; parse(...)

(but not import espree from "espree")

[nzakas]

@mdjermanovic import espree from "espree" would still work. You'd just need to use espree.parse().

[nzakas]

I think this is a good start and a great direction to head in. I just suggested one change to the build process, but otherwise, I'm in favor.

[nzakas]

I think what you're referring to here is the browserify step we have in the eslint repo? We use that just to create a browser-safe version of eslint for our demo and isn't an officially supported version of Espree. (You can't reference a UMD version from the Espree package.)

[nzakas]

I don't think we'd need to use import * as espree from "espree" based on the latest Node.js compat.

The second point is valid. Just adding exports to package.json is a breaking change.

[mdjermanovic]

import espree from "espree" would still work. You'd just need to use espree.parse().

Currently, import ... from 'espree' is getting the main CJS module, which is then interpreted as if it had export default module.exports; at the end.

When we add exports.import key to package.json, import ... from 'espree' will be getting the new ES module which, per the proof of concept PR, doesn't have export default. We should specify that in this RFC document.

It's similar to how import acorn from "acorn" works with acorn v7, but doesn't work with acorn v8.

[nzakas]

Yeah, I think that’s just an implementation detail that can easily be addressed.

[mdjermanovic]

The main module will have named exports (export function parse, export function tokenize...) and default export like export default { parse, tokenize, ... } so it could be used both ways?

[nzakas]

Yup. I think, for the purposes of this RFC, we should define how we want that to work and just accept that we'll probably have to prototype it out to make sure it works that way. I don't think we should take the prototype that preceded this RFC as requirements for how this should ultimately work.

[mdjermanovic]

@ljharb you mentioned here that there are some problems with the exports field on some Node 12, 13, and 14 versions?

[ljharb]

@mdjermanovic Yes, specifically, node v13.0 and v13.1 will only work with the string form; node v13.2-v13.6 will only work with the string form or the object form with a "default" key, and node v12.17+ and v13.7+ and v14+ work with all the features.

The solution, if you need an ESM export, is something like this:

"main": "./index.js"
"exports": {
  ".": [
    {
      "default": "./index.js"
    },
    "./index.js"
  ],
  "./foo": [
    {
      "default": "./foo.js"
    },
    "./foo.js"
  ]
}

The array fallback solves everything for the problematic nodes, and the similarity of "foo" to "foo.js" means pre-exports nodes will work, and "main" and the "." mean that the package name will work in both as well.

Essentially, if you list the paths you want to expose, I'm happy to write (or comment on a PR, or something) the "exports" field that would handle it.

[mreinstein]

Interesting https://blog.sindresorhus.com/get-ready-for-esm-aa53530b3f77

He's basically advocating for the pure es module solution.

[ljharb]

That's certainly a position, but:

1. You don't need type module at all - all that does is makes .js files be treated as ESM. You can and should use .mjs for ESM files, regardless.
2. it's still way too premature to avoid doing dual modules, and it's very hostile to look at excluding users as "ripping off a bandaid". I hope eslint does not make such a harsh choice.

[mreinstein]

it's still way too premature to avoid doing dual modules,

Yes, I agree. And in the article he mentions that this strategy won't be pursued until after node 10 sunsets (mid April.) Given that we'd (hopefully) have this live in eslint well before April, dual package is still a logical choice.

I was more speaking to it being genuinely interesting that someone influential is planning to go that route, not suggesting we should do that today. :)

[ljharb]

That author has often chosen to aggressively drop support for older environments by using modern syntax without transpiling, and as a result, many downstream users have suffered - either by being forced to switch to another (sometimes worse) dependency, or by being forced to upgrade environments sooner than required, or by passing the burden to their own users. I strongly encourage all package maintainers to avoid this pattern, influential actor or not.

[mreinstein]

I've updated the RFC with more details from the summary @btmills provided. The only outstanding item (I think) is what to do about the ESM usage. (providing dist/espree.mjs or instead just pointing at the code in lib/ directly.

Consistent with the assumption that ESM is the future standard path and that CJS affordances are temporary, I believe it would be better to just directly use that code without a build step. The idea has been that the build step is temporary and drops away when CJS becomes unnecessary.

[ljharb]

@mreinstein regardless of what you prefer to think of as the final case, or the primary authoring format, the graph duplication problem is real, and thus any stateful or identity-concerned code has to be authored in CJS (since you can import CJS, but can't require ESM).

[mreinstein]

has to be authored in CJS (since you can import CJS, but can't require ESM).

Maybe we're talking about different things. Obviously one cannot require esm, that's the entire point of producing a dual package. You need to provide entry points for both cjs and esm cases. There is no mandate on which format the source files must be in.

There's nothing stopping anyone from authoring in either format, and then producing whatever code needs to be pointed at in package.json to support both. And since either works, it just doesn't make sense to me to author in cjs since that's on the way out.

[ljharb]

@mreinstein right - but imagine, say, that eslint has a cache. That object needs to be shared by both CJS and ESM code in order to avoid the hazard. As such, a file exporting that object (or functions that close over it) must indeed be authored in CJS in order to be synchronously available to both module formats - even if the rest of the codebase is authored in ESM and transpiled to CJS.

[mreinstein]

Someone else is probably a better choice to move this forward, because I don't understand the use cases and the pitfalls affecting this system. I'd suggest someone else take this over.

[ljharb]

If the eslint team wants to list the exact entrypoints they want, and re-clarify that bundling is a necessity, and state a preference (or lack thereof) for authoring format, i'd be happy to prepare an implementation.

[nzakas]

While all of this background information is super interesting (thanks @ljharb), I think we're past the point where continuing the discussion along these lines is useful to progressing this RFC. There are any number of theoretical problems with publishing a dual package. The fact of the matter is that ESLint is the primary consumer of Espree, and we don't guarantee Espree for use in any other way, so most of the problems are unlikely to occur, and if they do, it will be our own fault (and we can fix them).

So in the spirit of moving this forward, I'm fine with shipping ESM source code as-is and then compiling a CJS file in dist for publishing. That's what @mreinstein originally proposed and we originally liked the idea of, so I've changed my mind and would like to let him run with it. If there end up being issues, it will be easier to solve them when the problem is concrete rather than trying to solve it in the abstract.

@mreinstein does that sound okay with you?

[mreinstein]

does that sound okay with you?

It sounds fine to me, but I'd like to put a little more color on my previous statement. I didn't mean to imply "it has to be my way or I'm taking my toys and going home." I'm coming from a place of being genuinely concerned about probably not understanding this particular module's use case enough, and that it might make more sense for someone that has that knowledge to be driving things.

[ljharb]

Sounds good. Still happy to help craft a maximally compatible "exports" field as needed.

[mreinstein]

Sounds good. Still happy to help craft a maximally compatible "exports" field as needed.

I will gladly take you up on that :-D

[nzakas]

@mreinstein no worries at all. I'm not sure anyone really understands this all that well, which is why there's been so much discussion.

Do you feel like the RFC currently represents your best and complete take on the approach?

[mreinstein]

Do you feel like the RFC currently represents your best and complete take on the approach?

I think so, but I'm happy to add more details to summarize what's been discussed in here if anyone thinks there are gaps. I very loosely addressed them in PR updates @ 1am this morning so I was even more prone to miss things than usual. :)

[aladdin-add]

there is a known issue related to webpack-bundled eslint: eslint/eslint#13974

it could also happen if we bundle eslint with rollup; but for espree, it's not an issue.

[nzakas]

Moving into final commenting as it seems we have our initial direction set at this point.

[btmills]

LGTM. This accurately reflects both the current state and where we want to go, and it answers all the discussion points. Thanks for all your work on this, @mreinstein. I'm excited to see this merged!

[ljharb]

type module is not required whatsoever; .mjs files are always ESM and .cjs files are always CJS, and type module makes .js mean ESM instead of CJS. for back compat (as well as ideological correctness), however, you actively do not want to change how .js files are parsed, so a dual package should either omit type or set it to "commonjs".

[mdjermanovic]

for back compat (as well as ideological correctness), however, you actively do not want to change how .js files are parsed, so a dual package should either omit type or set it to `"commonjs"

dist/espree.cjs will be the only CJS module we publish in v8. All .js files we're publishing as CJS in v7 will become ESM in v8, so we want them to be treated as ESM.

Not sure what's the plan for the files we use for development, like tests and tools.

[ljharb]

Right, but why not publish ESM as .mjs, rather than needing to change what .js has always meant?

[mreinstein]

rather than needing to change what .js has always meant?

I mean we're getting into opinion territory for sure, but I suspect that .mjs will eventually looked back upon as "oh yeah, that was that weird file extension proposal that node and google were advocating, back when the module ecosystem was super fragmented".

I think it's inevitable that what .js means is changing, that in the somewhat near future it will be assumed to be esm, and not the other way around.

[ljharb]

That may be true in the future, but that is not true now, and it seems like eslint should match the present instead of a possible future.

[mreinstein]

That may be true in the future, but that is not true now

I think that's a fair point 👍

eslint should match the present instead of a possible future.

I think it's a bit of a balancing act. It obviously needs to match present needs and expectations, but it also needs to prepare for a future that is coming pretty soon. esm has already shipped in browsers, in node, in deno.

[ljharb]

Sure, but browsers have no concept of extensions at all, and node is highly unlikely to ever change the default parsing behavior, so there's really no downside in sticking to the status quo for what extensions mean.

[ljharb]

i'd be careful about this; rollup tends to violate the spec in various ways. browserify works just fine with ESM as far as i'm aware.

[btmills]

We were previously only using browserify for a long-dead demo on the website. eslint/js#466 removes it. What spec violations should we be on the lookout for with rollup?

[ljharb]

I don't have any current ones in mind; but for one, its entire heuristic for removing dead code is flawed - it will remove things that can throw exceptions, which broke es5-shim on airbnb.com for many months, which was relying on those exceptions being triggered. They fixed this by detecting the specific cases es5-shim is using, but that doesn't fix the root problem. For another, when import * as can't be statically detected with certainty that some imports are unused, rather than keeping them all or erroring, they opt to drop them, so if you're doing anything fancy with module namespace objects, things can break.

I'm sure issues will be addressed as they're found, but the overall approach and ideology of the project keeps me very wary.

[mreinstein]

I'm sure issues will be addressed as they're found, but the overall approach and ideology of the project keeps me very wary.

The only thing we're really using any bundler for is changing the import to require and export to module.exports for the cjs build. I can't imagine there will be much spec violation if that's all we're doing?

browserify works just fine with ESM

huh, that's odd. My understanding is esm still is unsupported in browserify. Was looking to see if this has landed and it's still on the wishlist: browserify/browserify#1186

[ljharb]

To clarify, it works with ESM in concert with babel.

[mdjermanovic]

We'll bundle only our own code, so we probably don't need dead code removal. Can we turn it off with treeshake: false?

[mreinstein]

I'm pretty sure tree-shake is opt-in only; it's been experimental for as long as I know.

[mdjermanovic]

I think the docs for the treeshake option says Default: true.

[mreinstein]

oh those crazy bastards enabled it by default now! :D ok, then yeah we should definitely shut off tree-shake.

[nzakas]

I think we are at the point where we understand the overall direction. There are probably some details that will be easier to work out with code than opinion, so I think it’s safe to merge this. Any objections?

(To clarify: the RFC process is not intended to cover every edge case, it’s to describe and approve a general direction to head in.)

[nzakas]

Okay, let's get to work on this thing. :)