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

New: Provide ESM package for Espree #72

Merged
merged 7 commits into from Feb 17, 2021
170 changes: 170 additions & 0 deletions designs/2020-es-module-support/README.md
@@ -0,0 +1,170 @@
- Repo: eslint/espree
- Start Date: 2020-12-13
- RFC PR: https://github.com/eslint/rfcs/pull/72
- Authors: Mike Reinstein (https://github.com/mreinstein)

# Adding es module support

## Summary

Provide an ecmascript module for espree.


## Motivation

Historically there have been a number of competing module standards: commonjs, requireJs, iife wrapped script tags, etc.

This has contributed to a very complicated tooling scenario for node/npm/javascript based projects in general:
* every module has it's own unique build system, with no consistency across modules.
* projects are packed with complex configurations, much of which is plumbing related to packaging/bundling.

Over time, browsers have adopted the ecma module standard. Today, support is pretty good:
https://caniuse.com/es6-module

Node also added support in v12.17.1 and up, and has officially marked the API stable:
https://nodejs.org/dist/latest-v15.x/docs/api/esm.html#esm_modules_ecmascript_modules


By adopting this new standard, we can achieve a few things:

* eventual simplification of espree's build process by eliminating the need for multiple module formats
* make it easier to directly importing espree in browser environments, deno, etc. Skypack is a CDN that already enables this, but they have internal logic that looks at every package in npm and tries to build an es module. https://www.skypack.dev/

As more projects adopt this standardized module format, the hope is a lot of this "connective glue" will fall away, leaving us with leaner modules and less build tooling across our stack.


## Detailed Design

### Nomenclature

* `ESM` ecmascript module. The standardized module format for javascript
* `CJS` commonjs. Node's historical module format (i.e., `module.exports = ...`, `const a = require('foo')`)
* `UMD` universal module definition. A legacy format for specifying javascript code as a module in a way that tries to guarantee compatibility with many loader formats including requireJS, commonjs, and global script tags.
* `IIFE` immediately invoked function expressions. A legacy strategy for encapsulating javascript into a module that won't leak global variables into the execution environment. It is still the primary way of packaging global script tags for inclusion in a web page. `UMD` is usually wrapped in an `IIFE`
* `CDN` content delivery network. can be thought of as a set of servers that host static content all around the globe to deliver assets quickly and at scale.


### build process

Today, espree is written in commonjs (CJS) format, and a universal module definition (UMD) build is created too.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

mreinstein marked this conversation as resolved.
Show resolved Hide resolved


today's build process:
```
┌-------------------┐ ┌-----------------┐
│ │ │ │ package.json:
│ espree.js | │ build/espree.js │
│ (CJS entry point) ├──BUILD_STEP──▶│ (UMD bundle) │ CJS ---▶ "main": "espree.js",
│ │ │ │
└-------------------┘ └-----------------┘
```


The strategy that probably makes the most sense in this initial work is to do what is known as a "dual package" solution, where
espree provides an ESM, and a CJS module.

Providing a dual package shouldn't affect any existing espree users, but provies an ESM option for users that want it.


proposed build process in this RFC:
nzakas marked this conversation as resolved.
Show resolved Hide resolved
```
┌-------------------┐
│ │
│ dist/espree.cjs │
│ (CJS entry point) │ package.json:
┌-------------------┐ │ │
│ │ └--▲----------------┘ CJS ---▶ "main": "dist/espree.cjs",
│ espree.js │ │ "exports": {
│ (ESM entry point) ├───BUILD_STEP───┘ ESM ---▶ "import": "espree.js",
│ │ CJS ---▶ "require": "./dist/espree.cjs"
└-------------------┘ },
"type": "module"
Copy link
Sponsor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Sponsor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Sponsor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Sponsor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

```

browserify is specifically oriented around commonjs as the input format, so I also propose replacing it with rollup, which understands and expects ESM as it's input format.
Copy link
Sponsor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Sponsor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Sponsor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


`UMD` is not part of the formal package interface and will be dropped altogether.


## Documentation

The changes should be described in the migration guide for whichever major version of espree this goes into (currently considering the upcoming 8.x line.)

Adding an `exports` field to `package.json` will break the existing API, it makes sense to formally announce this via blog post.


## Drawbacks

The javascript build/tooling ecosystem is already monstrously complex, and this change adds another output format, which further complicates things (compare the 2 graphs above to see this additional complexity visualized.)

I do believe this is a temporary scenario; as time goes on, the UMD and CJS entry points could be dropped altogether, along with the build step. But that will take months, maybe even years depending on how comfortable the community is with this change and how widely adopted the ESM format becomes.


## Backwards Compatibility Analysis

The dual package approach _should_ not break espree for anyone currently using espree.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



## Alternatives

Another option is to just drop CJS, UMD and the build system altogether, provide an ESM only implementation,
and make this a part of a semver-major version bump. (e.g., espree@8 is ESM only.)

This would essentially eliminate the build step and make things very simple, but it has a big downside:
* users that don't support ESM only projects would be stuck on `espree <= 7.x` forever (or at least until they added the ability to use ESM in their own stuff.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems not a big deal:

  • node v10 is going to be EOL in 2021-04-30, then we can drop node <12, and rewrite the codebase to es modules.

Copy link
Sponsor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't do that; converting things to ESM means they can't be required at all. (note, you'd also have to drop node < 12.17, and < 13.7).

The best practice remains writing everything in CJS for the foreseeable future, and writing thin ESM wrappers as needed to provide named exports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aladdin-add given how widely depended upon eslint stuff is, people here tend to be (rightly so) more conservative in adopting these big changes. I think what you're describing is a totally valid approach for new modules, and it's definitely the strategy I'm using in my own personal module work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb we can provide a compiled version (esm to cjs using something like babel), and I opened an issue about dropping node versions: eslint/eslint#14023

this helps eslint developers writing esm, rahter than continuing using cjs - cjs will go away finally. :)

Copy link
Sponsor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CJS will never go away.

Certainly you can author in either format and transpile to the other, but i fail to see the advantage of that over just authoring CJS and ESM wrappers without a build step. CJS is precisely as tree-shakeable as ESM, and for node 12 there’s no syntactic advantage either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CJS will never go away.

Yeah, and 640k of memory ought to be enough for anybody. ;)

Copy link
Member

@btmills btmills Jan 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we discussed this topic in a recent TSC meeting, we decided to go with a dual package approach to allow a gradual migration. On an infinite time scale we’ll likely go ESM-only, but I wouldn’t want to speculate on a timeline for that.


## Open Questions

None yet.


## Help Needed

None yet.


## Frequently Asked Questions

> what is this "dual package" stuff?

It's a proposal by the node community on how a module may go about adopting ESM without totally breaking CJS for their users. https://nodejs.org/api/packages.html#packages_writing_dual_packages_while_avoiding_or_minimizing_hazards


> are any other well known modules using this approach?

acorn is producing a dual module, and has been the inspiration for my own work around the forthcoming espree PR https://github.com/acornjs/acorn/blob/master/acorn/package.json#L5-L11


> does this mean local development now requires a build step?

no. Tests should be written to run against the ESM source in `lib/`. The pre-release tests should be run against the CJS bundle as part of the pre-publish step.


> how would one import espree now that it's providing an esm interface?

There will be named exports. i.e.,

```javascript
import { parse, tokenize /*, ... */ } from 'espree';
```


> npm modules reference each other via commonjs, and the browser's loader expects URLs, so why are we concerned with making npm modules work in a browser?

that's true, you can't simply do `import espree from 'https://github/eslint/espree.js'` naively because it's referencing other npm modules in it, such as `acorn`.

However there are some CDNs which will transform npm modules to refer to URLs. For example, this works today:

```javascript
import espree from 'https://cdn.skypack.dev/espree'
```

By making espree a standard ESM it reduces the amount of transforms that need to be run on it to make it usable.

It's also possible that in the semi-near future, node may consider offering URL loaders. Deno is already doing this.


## Related Discussions

- [1] The discussion genesis https://github.com/eslint/espree/issues/457