Breaking: Switch to ESM (fixes #70)

[brettz9]

Breaking: Switch to ESM

Fixes #70.

[eslint-github-bot]

Hi @brettz9!, 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 commit message tag must be one of the following:

  The Tag is one of the following:

  • Fix - for a bug fix.
  • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
  • New - implements a new feature.
  • Breaking - for a backwards-incompatible enhancement or feature.
  • Docs - changes to documentation only.
  • Build - changes to build process only.
  • Upgrade - for a dependency upgrade.
  • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

  You can use the labels of the issue you are working on to determine the best tag.

• There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

[eslint-github-bot]

Hi @brettz9!, 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 commit message tag must be one of the following:

  The Tag is one of the following:

  • Fix - for a bug fix.
  • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
  • New - implements a new feature.
  • Breaking - for a backwards-incompatible enhancement or feature.
  • Docs - changes to documentation only.
  • Build - changes to build process only.
  • Upgrade - for a dependency upgrade.
  • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

  You can use the labels of the issue you are working on to determine the best tag.

• There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

[eslint-github-bot]

Hi @brettz9!, 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 commit message tag must be one of the following:

  The Tag is one of the following:

  • Fix - for a bug fix.
  • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
  • New - implements a new feature.
  • Breaking - for a backwards-incompatible enhancement or feature.
  • Docs - changes to documentation only.
  • Build - changes to build process only.
  • Upgrade - for a dependency upgrade.
  • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

  You can use the labels of the issue you are working on to determine the best tag.

• There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

[eslint-github-bot]

Hi @brettz9!, 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 commit message tag must be one of the following:

  The Tag is one of the following:

  • Fix - for a bug fix.
  • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
  • New - implements a new feature.
  • Breaking - for a backwards-incompatible enhancement or feature.
  • Docs - changes to documentation only.
  • Build - changes to build process only.
  • Upgrade - for a dependency upgrade.
  • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

  You can use the labels of the issue you are working on to determine the best tag.

• There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

[brettz9]

Am I still getting commit message errors because I've more than one commit in the PR?

[nzakas]

Yes, when there's more than one commit then it checks the pull request title. I fixed it.

[nzakas]

We will need to update the Node.js testing matrix to match the 12/14/16 that we have in Espree and eslint-visitor-keys to get everything working.

Thanks again for taking this on!

[brettz9]

We will need to update the Node.js testing matrix to match the 12/14/16 that we have in Espree and eslint-visitor-keys to get everything working.

Updated.

Thanks again for taking this on!

My pleasure. Really like ESM both for it being a universal syntax and conducing to polyglot browser-friendly, build-free code. Also eager to get closer to using ESM for plugins, etc.

[nzakas]

I think this looks good. I'd like to merge #69 first and then rebase this on top of that before merging.

[nzakas]

#69 has been merged, so when you feel up to rebase on top of that, we can look at this again.

[nzakas]

Where we are: it seems that the subpaths being used by typescript-eslint and Webpack aren't required going forward, so we don't need to provide any subpath exports for those.

[nzakas]

@nicolo-ribaudo it looks like Babel’s ESLint parser is reaching into eslint-scope to access some undocumented APIs that will no longer be accessible in the next release. Can you help us understand what the code is doing and how removing access to these APIs will affect you?

https://github.com/babel/babel/blob/9d620c2d423cec4cf5d555d4b8884288068bd17d/eslint/babel-eslint-parser/src/analyze-scope.cjs#L1-L4

[sokra]

@sokra this package will still export a CommonJS entrypoint, so hopefully that will help.

That helps. As long main points to the commonjs version, old node.js versions are happy.

Also see our great guide for the exports field: https://webpack.js.org/guides/package-exports/

[bradzacher]

@nzakas on behalf of the babel folks as I'm pretty sure I know what they are doing.

They're extending this scope analyser to add support for the additional syntax they support.

We at ts-eslint used to do the same thing, until we bit the bullet and forked/rewrote it due to our requirement to support the separate value+type scope analysis.

Babel does technically support TS, though the vast, vast majority of users use ts-eslint for that usecase. So instead babel-eslint serves people who are using pure JS with additional features not yet supported by espree / this project. Meaning their extension is usually fairly limited in comparison.

If you remove that access then it becomes hard for them to extend this package to add support for additional syntax without maintaining a complete fork of the project.

They can correct me if I'm wrong though :)

[nicolo-ribaudo]

Thanks @bradzacher, it's 100% correct.

@nzakas If those internal APIs will be made inaccessible from the outside, we will probably fork eslint-scope and maintain those visitors ourselves. However, it will be more likely for us to accidentally fall behind ESLint causing potential problems for @babel/eslint-parser users.

Btw, in Babel 8 we'll directly import eslint-scope using ESM rather than CJS.

(cc @hzoo who has more experience than me in our ESLint integration)

[hzoo]

Not much else to add to what @bradzacher/@nicolo-ribaudo said, this is some code we've had for a while, (this was the last PR babel/babel-eslint#542) but the general idea has been there since the beginning to monkey patch /updating scopes for new ast features/types/differences in the ast.

[nzakas]

@nicolo-ribaudo @hzoo Hmmm, okay. I don't think this is a great approach (any more than using undocumented APIs is ever a good idea), but we definitely don't want to break you.

Long-term I think we need to reimagine how this package works. If everyone keeps using it as a base to extend, then we should really design an API that allows for that in an official way rather than coming across one-off uses that are using private methods.

TSC Summary: This PR switches eslint-scope to be an ESM package. We had originally planned on removing access to APIs under lib/, but that will break @babel/eslint-parser.

TSC Question: How shall we proceed?

So I think we have to decide how to move forward, which I see as one of:

1. "Bless" these APIs and just export them directly from this package.
2. Create a use-at-your-own-risk entrypoint like we did for ESLint that exports the things we don't want to officially support.
3. Keep all of the current APIs available at the current locations.

[btmills]

Good news: in the most recent TSC meeting, we agreed to export the APIs under lib/ as public APIs. (Option 1 above.)

[brettz9]

We are currently giving both import and require options for the main lib/index.js export, but does this new decision mean we need to add a Rollup process for each of the subfiles, or is the idea just to expose those import style and let consumers do any bundling/transpiling to CJS themselves?

[brettz9]

I.e., adding 16 vs. 8 lines for each one of the additional lib files (or 14 vs. 7 if dropping version.js since it can be obtained by the package.json which is already ready to be exported).

Also, of those files, the exported files might be reduced further if those already being exported in full by index.js are removed, i.e., scope-manager.js, reference.js, and variable.js.

[mdjermanovic]

We are currently giving both import and require options for the main lib/index.js export, but does this new decision mean we need to add a Rollup process for each of the subfiles, or is the idea just to expose those import style and let consumers do any bundling/transpiling to CJS themselves?

Hmm, my understanding of option 1 was that lib/index.js will re-export Referencer, PatternVisitor, and Definition, but I could be wrong.

[nzakas]

Hmm, my understanding of option 1 was that lib/index.js will re-export Referencer, PatternVisitor, and Definition, but I could be wrong.

This is correct. We would like lib/index.JS to export ScopeManager, Scope, Referencer, Definition, etc. (Everything Babel/eslint-parser is using) So we would still end up with one file created by Rollup and you shouldn’t need to update the Rollup script at all.

[brettz9]

Ah, ok, that makes more sense :-) Added a commit with the exports...

[nzakas]

LGTM. Thanks for all our your work on this.

[mdjermanovic]

A problem with this is that our release process doesn't commit lib/version.js, so after the 6.0.0 release we'll still have const version = "5.1.1"; in lib/version.js.

[mdjermanovic]

This would then cause the next release (e.g. 6.1.0) to fail: npm install will trigger the prepare script, which will overwrite lib/version.js with 6.0.0. Then, npm version will throw error "Git working directory not clean".

[nzakas]

I’ll fix this in a separate PR.

[btmills]

Just one quick inline suggestion. Thanks for all the work converting another package!

[mdjermanovic]

LGTM assuming we'll align the scripts with our release process in a separate PR.

Thanks, @brettz9!

[nzakas]

Thanks everyone, I'll merge this and start work on the release scripts. Awesome work @brettz9