Breaking: provide ESM export (refs eslint/rfcs#72) #469

mreinstein · 2021-02-23T00:55:22Z

here's the new attempt, which attempts to satisfy all of the criteria listed in https://github.com/eslint/rfcs/tree/master/designs/2020-es-module-support

There are a ton of changes in here, but the bulk of them are trivial changes to test/fixtures/* which convert from cjs to esm.

mreinstein · 2021-02-23T01:00:17Z

the tests are breaking because Makefile.js is gone. Whatever build process is running on the CI infrastructure would prob need to be updated. The current test steps are npm test, which will run all of the existing linting and unit tests.

nzakas

Looks good! Just a few comments. Also, the reference to Makefile.js is here:

https://github.com/eslint/espree/blob/master/.github/workflows/ci.yml#L22

lib/version.js

rollup.config.js

package.json

eslint-github-bot · 2021-02-28T00:37:15Z

Hi @mreinstein!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

The issue reference must be formatted as follows:

If the pull request addresses an issue, then the issue number should be mentioned at the end. If the commit doesn't completely fix the issue, then use (refs #1234) instead of (fixes #1234).

Here are some good commit message summary examples:
```
Build: Update Travis to only test Node 0.10 (refs #734)
Fix: Semi rule incorrectly flagging extra semicolon (fixes #840)
Upgrade: Esprima to 1.2, switch to using comment attachment (fixes #730)
```

Read more about contributing to ESLint here

eslint-github-bot · 2021-02-28T00:40:11Z

Hi @mreinstein!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

The issue reference must be formatted as follows:

If the pull request addresses an issue, then the issue number should be mentioned at the end. If the commit doesn't completely fix the issue, then use (refs #1234) instead of (fixes #1234).

Here are some good commit message summary examples:
```
Build: Update Travis to only test Node 0.10 (refs #734)
Fix: Semi rule incorrectly flagging extra semicolon (fixes #840)
Upgrade: Esprima to 1.2, switch to using comment attachment (fixes #730)
```

Read more about contributing to ESLint here

eslint-github-bot · 2021-02-28T00:47:55Z

Hi @mreinstein!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

The issue reference must be formatted as follows:

If the pull request addresses an issue, then the issue number should be mentioned at the end. If the commit doesn't completely fix the issue, then use (refs #1234) instead of (fixes #1234).

Here are some good commit message summary examples:
```
Build: Update Travis to only test Node 0.10 (refs #734)
Fix: Semi rule incorrectly flagging extra semicolon (fixes #840)
Upgrade: Esprima to 1.2, switch to using comment attachment (fixes #730)
```

Read more about contributing to ESLint here

mreinstein · 2021-02-28T00:53:15Z

Are we planning to continue supporting node 10.x in the testing matrix? I didn't realize we were testing that far back. An issue with that is the tests have all been updated to use esm and won't work with that version of node.

mdjermanovic · 2021-02-28T16:08:12Z

Are we planning to continue supporting node 10.x in the testing matrix?

We should have tests on node 10.x since it's a supported version, but we can run there only tests against the CJS bundle.

.eslintrc.cjs

mdjermanovic · 2021-02-28T17:02:50Z

rollup.config.js

+// the espree package version is exported from the module (i.e., import { version } from "espree")
+// read version from the package.json metadata and write it into lib/version.js at build time,
+// since esm cannot import json by default
+const pkg = JSON.parse(fs.readFileSync("./package.json", "utf8"));
+fs.writeFileSync("lib/version.js", `const version = "${pkg.version}";\n\nexport default version;\n`);


Is this a common practice? We should revisit our release steps to make sure that package.json has been updated before running this code.

I don't know if it's common practice or not. Personally I don't see why the version number even needs to be explicitly exported from the module at all, but this seemed like an easy way to accomplish that without adding even more plugins and tooling.

I think this is fine for now. In the future, Node.js will support JSON imports and we'll be able to remove this.

We should modify our release process.

Current steps in generateRelease are:

npm test

npm version to update package.json

But we should have this order now:

npm version to update package.json

npm run rollup to make lib/version.js and dist/espree.cjs.

npm test

Maybe we could reorder the steps in generateRelease (which is used for all packages, not just espree) to calculate the new version and update package.json before running npm test, and also update the test script to run rollup before the tests.

In the current esm PR, npm run rollup implicitly updates lib/version.js.

I did it this way because I don't know of any use cases where one would want to update version and not re-package the commonjs build in dist/.

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

tests/lib/acorn-after-espree.js

mdjermanovic · 2021-02-28T22:10:36Z

Can we add a script that will run all tests against dist/espree.cjs? Maybe we could use babel and @babel/register to transform imports and exports to CJS code.

package.json

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

mreinstein · 2021-02-28T22:29:31Z

Can we add a script that will run all tests against dist/espree.cjs?

Ehh maybe? I don't know how that would work though. All the tests are in esm with this PR.

Maybe we could use babel and @babel/register to transform imports and exports to CJS code.

blech... :(

mreinstein · 2021-02-28T22:30:51Z

I would be more inclined to just say the hell with node 10, it goes out of maintenance in like 7 weeks. :P

mdjermanovic · 2021-02-28T22:37:24Z

I would be more inclined to just say the hell with node 10, it goes out of maintenance in like 7 weeks. :P

I didn't mean that only because of running tests on Node 10. CJS is still the main output (if nothing else, ESLint will use that entry point), we should be sure that it works.

mreinstein · 2021-02-28T22:43:03Z

I just modified one of the tests to point at the cjs build:

import "../../dist/espree.cjs";

This works ☝️

I guess it's a question of how to do this. Should all of the tests point at cjs, or is sufficient to just have a handful of them using the cjs build?

mdjermanovic · 2021-02-28T22:54:37Z

If there is no other option, I'd vote that all tests work with dist/espree.cjs (in which case the tests could be CJS), but ideally some tool could help us to transform the tests and run them on both ESM and CJS versions of espree.

mreinstein · 2021-02-28T23:07:43Z

Probably the right thing to do would be to create a series of tests that explicitly touch all parts of the public API that is exported, and asssert they are still accessible via cjs. Otherwise we are running a ton of duplicate tests, doubling the testing time, and over-complect-ifying the process.

I think we really just need to assert that the public portion of the interface is still accessible, since that's all that is being changed.

mreinstein · 2021-03-02T02:42:57Z

apart from node 10.x failing in CI, all the unit tests and lint rules should pass now.

mreinstein · 2021-03-09T22:05:31Z

ok, seems all the tests at least pass now. I think the summary of remaining tasks that @mdjermanovic listed are pretty good, but I'll focus on two that are probably the most controversial and still need to be figured out:

tap vs mocha Some conflicting suggestions here, improve the tap reporter vs try to make mocha work. I'm definitely not keen to kill mocha, I just couldn't seem to get it to work with dynamic imports. I'm pretty sure when I spent that full day trying this I had upgraded to mocha@8, but I can try again I suppose, just to be sure. I recall something about handling dynamic imports with mocha by using babel to transpile tests, but this just seems really awful to me. For one we're adding a lot more ceremony to the tests, and kind of altering the tests subtley under the hood. I can see if there's some way to make mocha work without this extra babel step.
run all tests against cjs vs just the entry points of cjs. I'm not really opposed to either, though again I do think we're creating a lot of test duplication, and adding more ceremony by needing some process to run everything through both cjs and esm builds. I'm sure it's doable but it's unclear to me how to go about that in a way that doesn't suck. Open for suggestions.

mdjermanovic · 2021-03-10T14:54:30Z

I tried with the latest mocha 8.3.1: https://github.com/mdjermanovic/espree/commit/8677c802e26a25bd714413b0705e7b89e7454d1f

It worked locally on Node 12, but had to fix a CI issue with peer deps on Node 15 (leche requires mocha < 7 although it seems to be working well with the latest mocha too): https://github.com/mdjermanovic/espree/commit/17e508230291d518d219bd71737e288f4efcd22f

Here's a repo with those two commits on top of the latest commit from this branch: https://github.com/mdjermanovic/espree

I think it works, all Node 12+ runs show 1300 passing ESM tests. We currently have 1299 passing on master, and 1 was added in this PR? (+ 7 CJS tests)

Coverage doesn't work, but it doesn't work with tap as well?

mreinstein · 2021-03-12T05:13:45Z

I think it works, all Node 12+ runs show 1300 passing ESM tests.

Ah ok, well if it works we should use that I guess, since it's technically less change vs switching everything to tap.

mreinstein · 2021-03-13T17:38:14Z

@mdjermanovic I tried using the exact changes in your branch, and it fails for me locally:

espree on  master [!] is 📦 v7.3.1 via ⬢ v14.16.0
➜ npm run unit:esm

> espree@7.3.1 unit:esm /Users/mike/Sites/espree
> nyc mocha --color --reporter progress --timeout 30000 'tests/lib/**/*.js'

/Users/mike/Sites/espree/node_modules/yargs/yargs.js:1172
      else throw err
           ^

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /Users/mike/Sites/espree/tests/lib/acorn-after-espree.js
require() of ES modules is not supported.
require() of /Users/mike/Sites/espree/tests/lib/acorn-after-espree.js from /Users/mike/Sites/espree/node_modules/mocha/lib/mocha.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename acorn-after-espree.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /Users/mike/Sites/espree/package.json.

mreinstein · 2021-03-13T17:43:23Z

oh, it does work! It turns out that npm update wasn't sufficient and things were still being cached. when blasted out node_modules and re-installed, the tests work.

mreinstein · 2021-03-13T18:21:42Z

regarding updating the tools/ directory, something to keep in mind is some of these script were already broken in the main branch. e.g., running node tools/generate-identifier-regex.js is failing with

Error: Cannot find module 'unicode-6.3.0/categories/Lu/code-points'

because these files don't seem to be in unicode-6.3.0@0.7.5. Other tool scripts are failing too.

nzakas · 2021-03-17T00:44:59Z

@mreinstein can you open an issue for the tool problem? I’d like to make sure we don’t lose track of that, and it’s probably not helpful to bundle with this PR.

nzakas · 2021-03-27T00:42:10Z

@mdjermanovic can you take a look to see if your concerns are addressed?

mdjermanovic · 2021-04-11T23:17:22Z

We discussed the remaining tasks, and decided to:

Not run all tests on .cjs bundle, at least for now.
Update release process in a separate PR (issue espree v8 release process #475).
Update documentation in a separate PR (issue ESM API docs #474).

There's already an open issue #471 to update tools/ scripts in a separate PR.

mreinstein · 2021-04-11T23:31:16Z

hey, sorry to drop off the planet face for a while. Thanks for the status update.

What are the remaining things that you'd like me to work on to get this PR into a completed state?

mdjermanovic

Sorry for the delay in reviewing!

I have only two small suggestions. One is to update lint scripts in package.json (I left the suggested change). The other is to add a test with some JSX code to tests/lib/commonjs.cjs.

Everything else LGTM

package.json

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

mdjermanovic

LGTM, thanks for all the work!

nzakas · 2021-04-16T17:25:59Z

All right, let's get this merged so we can move forward. 🎉

Build: generate es and cjs modules. Fixes #457

e6bc8c7

mreinstein mentioned this pull request Feb 23, 2021

Build: es module proof of concept DO NOT MERGE :) fixes #457 #458

Closed

nzakas requested changes Feb 25, 2021

View reviewed changes

lib/version.js Show resolved Hide resolved

rollup.config.js Outdated Show resolved Hide resolved

rollup.config.js Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

Build: add note explaining lib/version.js and lint

0cb01c0

This comment has been minimized.

Sign in to view

Build: update unit and lint testing commands

e529adc

mdjermanovic changed the title ~~Build: generate es and cjs modules. Fixes #457~~ Breaking: provide ESM export (refs eslint/rfcs#72) Feb 28, 2021

This comment has been minimized.

Sign in to view

mdjermanovic requested changes Feb 28, 2021

View reviewed changes

.eslintrc.cjs Outdated Show resolved Hide resolved

mdjermanovic reviewed Feb 28, 2021

View reviewed changes

Update .eslintrc.cjs

b65da90

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

mdjermanovic reviewed Feb 28, 2021

View reviewed changes

tests/lib/acorn-after-espree.js Show resolved Hide resolved

mdjermanovic requested changes Feb 28, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

Update package.json

14f33cd

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

mreinstein added 2 commits March 1, 2021 18:32

Chore: add commonjs unit tests and lint (fixes #469)

195a7f2

Chore: fix acorn unit test and lint (fixes #469)

9e82484

mreinstein added 3 commits March 9, 2021 12:22

Chore: use pathToFileURL to hopefully fix esm based tests on windows

2b3f5d9

Chore: use pathToFileURL to hopefully fix esm based tests on windows

cbbf850

Chore: use pathToFileURL to hopefully fix esm based tests on windows

30bd6e6

replace tap with mocha and minor dep updates

a4df258

Fix Node 15 peer deps issue

8ab5181

mreinstein mentioned this pull request Mar 17, 2021

Fix broken scripts in tools/ and convert to esm #471

Closed

This was referenced Apr 11, 2021

no-redeclare loose parser tests eslint/eslint#14315

Closed

Breaking: acorn to 8.1.0 (fixes #470) #473

Merged

ESM API docs #474

Closed

espree v8 release process #475

Closed

mdjermanovic requested changes Apr 12, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

mreinstein and others added 3 commits April 11, 2021 21:04

Update package.json

f3af763

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

fix linting problems

6a43c72

add basic jsx test for commonjs build

10dc394

nzakas approved these changes Apr 14, 2021

View reviewed changes

mdjermanovic approved these changes Apr 15, 2021

View reviewed changes

mdjermanovic added accepted breaking labels Apr 15, 2021

nzakas merged commit 8234c48 into eslint:master Apr 16, 2021

Breaking: provide ESM export (refs eslint/rfcs#72) #469

Breaking: provide ESM export (refs eslint/rfcs#72) #469

Conversation

mreinstein commented Feb 23, 2021

mreinstein commented Feb 23, 2021

nzakas left a comment

Choose a reason for hiding this comment

eslint-github-bot bot commented Feb 28, 2021

eslint-github-bot bot commented Feb 28, 2021

This comment has been minimized.

eslint-github-bot bot commented Feb 28, 2021

mreinstein commented Feb 28, 2021

This comment has been minimized.

mdjermanovic commented Feb 28, 2021

mdjermanovic Feb 28, 2021

Choose a reason for hiding this comment

mreinstein Feb 28, 2021

Choose a reason for hiding this comment

nzakas Mar 2, 2021

Choose a reason for hiding this comment

mdjermanovic Mar 3, 2021

Choose a reason for hiding this comment

mreinstein Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

mdjermanovic commented Feb 28, 2021

mreinstein commented Feb 28, 2021

mreinstein commented Feb 28, 2021

mdjermanovic commented Feb 28, 2021

mreinstein commented Feb 28, 2021

mdjermanovic commented Feb 28, 2021

mreinstein commented Feb 28, 2021

mreinstein commented Mar 2, 2021

mreinstein commented Mar 9, 2021

mdjermanovic commented Mar 10, 2021

mreinstein commented Mar 12, 2021

mreinstein commented Mar 13, 2021

mreinstein commented Mar 13, 2021

mreinstein commented Mar 13, 2021 • edited Loading

nzakas commented Mar 17, 2021

nzakas commented Mar 27, 2021

mdjermanovic commented Apr 11, 2021 • edited Loading

mreinstein commented Apr 11, 2021

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic left a comment

Choose a reason for hiding this comment

nzakas commented Apr 16, 2021

mreinstein Mar 17, 2021 •

edited

Loading

mreinstein commented Mar 13, 2021 •

edited

Loading

mdjermanovic commented Apr 11, 2021 •

edited

Loading