-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
Changes from all commits
de5b2ec
6758ec6
1d18c2c
c43b807
c6d3e66
e33981e
901d97f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. | ||
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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. type module is not required whatsoever; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not sure what's the plan for the files we use for development, like tests and tools. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, but why not publish ESM as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I mean we're getting into opinion territory for sure, but I suspect that I think it's inevitable that what There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think that's a fair point 👍
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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/js#466 removes it. What spec violations should we be on the lookout for with rollup? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I'm sure issues will be addressed as they're found, but the overall approach and ideology of the project keeps me very wary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The only thing we're really using any bundler for is changing the
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify, it works with ESM in concert with babel. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the docs for the treeshake option says There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe there will be no difference for Node apps/libs that
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we'd need to use The second point is valid. Just adding |
||
|
||
|
||
## 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.) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems not a big deal:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The best practice remains writing everything in CJS for the foreseeable future, and writing thin ESM wrappers as needed to provide named exports. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, and 640k of memory ought to be enough for anybody. ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)