Breaking: Switch to ESM

[brettz9]

Fixes #22.

Also:

• Refactor(lint): Fix Object.prototype.hasOwnProperty call as per latest linting requirements
• Chore(lint): Adjust .eslintrc.json to pass linting of modules; uses ES2021 to support import.meta.url
• Chore(editorconfig): Add .editorconfig
• Chore(npm): Update devDeps.
• Chore(npm) Switches from nyc to c8 due to nyc requiring extra loader: Nyc on ESM Node.js 13 (no babel) istanbuljs/nyc#1287

Using synchronous fs API since top-level await not yet supported (nor is ESM importing of JSON well-supported).

(I'm not sure why in my IDE, Atom, I'm seeing node/no-unsupported-features/es-syntax errors but in the lint script I am not.)

[eslint-github-bot]

Hi @brettz9!, thanks for the Pull Request

The first commit message 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.

Read more about contributing to ESLint here

[brettz9]

If I can get feedback about whether I'm on the right track with this, I may be able to also look at eslint/eslint-scope#70 and eslint/eslintrc#35 .

[nzakas]

This LGTM. I’d like another set of eyes on it before merging.

Thanks for doing this!

[aladdin-add]

LGTM, thanks!

[mdjermanovic]

We already have one ESM project: https://github.com/eslint/espree. Ideally, all repositories should be aligned with espree repo in that regard.

[brettz9]

@mdjermanovic : Do the issues you mentioned comprise the alignment changes you still feel or needed, or were there any other dimensions where you wanted alignment? I've submitted some of the other changes I aaw.

Note that I didn't align with nyc because that is broken with ESM, and I didn't align the rollup script in JSON to specify rollup.config.js as that seems redundant.

Btw, you are using prepublishOnly to invoke the build script while @nzakas mentioned a prepare script. Not sure which you want.

[brettz9]

Btw, per my alignment with espree, I didn't see much mention of the array you used with exports, though I do see mention of a fallback array in https://nodejs.org/api/packages.html (though nothing else). But I'll take your word it works. :-)

[mdjermanovic]

@mdjermanovic : Do the issues you mentioned comprise the alignment changes you still feel or needed, or were there any other dimensions where you wanted alignment? I've submitted some of the other changes I aaw.

Thanks for the changes! I'll check everything again and let you know if there is anything left.

Note that I didn't align with nyc because that is broken with ESM

That's great, coverage is currently broken in the espree repo.

and I didn't align the rollup script in JSON to specify rollup.config.js as that seems redundant.

👍 that's fine

Btw, you are using prepublishOnly to invoke the build script while @nzakas mentioned a prepare script. Not sure which you want

Espree had an additional complexity with update-version. prepare should be fine for this project, and it's also simpler and better for various reasons.

Btw, per my alignment with espree, I didn't see much mention of the array you used with exports

That was discussed in the RFC. It's for compatibility with some Node versions, though I'm not sure if we need that anymore after #23.

[brettz9]

Ok, cool.

As far as the fallback array, since it seems less documented and more verbose, maybe it is better to avoid it if the version support mentioned in eslint/rfcs#72 (comment) are accurate. Let me know if you want to keep it or revert (and just add the "default" key).

[nzakas]

Regarding prepare vs prepublishOnly: in general we want to use prepare everywhere. Espree had a special case where that wouldn’t work.

[mdjermanovic]

Linting fails due to missing plugins required by the new eslint-config-eslint. You can add eslint-plugin-node and eslint-plugin-jsdoc.

[brettz9]

It would be nice to have a small test file that would require("../../dist/eslint-visitor-keys.cjs"), and then check e.g. if getKeys is a function etc., similar to this one:

https://github.com/eslint/espree/blob/main/tests/lib/commonjs.cjs

Added.

[mdjermanovic]

It might be more suitable to do this freezing in visitor-keys.js now?

eslint-visitor-keys/lib/index.js

Lines 12 to 16 in 7279e73

	// Freeze the keys.
	for (const type of NODE_TYPES) {
	Object.freeze(KEYS[type]);
	}
	Object.freeze(KEYS);

[brettz9]

It might be more suitable to do this freezing in visitor-keys.js now?

eslint-visitor-keys/lib/index.js

Lines 12 to 16 in 7279e73

	// Freeze the keys.
	for (const type of NODE_TYPES) {
	Object.freeze(KEYS[type]);
	}
	Object.freeze(KEYS);

Done.

[mdjermanovic]

Does npm run coverage works for you? I'm getting some errors.

[brettz9]

Looks like it is this reported bug: bcoe/c8#184 . It had been working earlier.

(Logging shows that there is a "branchMap" expected but it only has keys 0-6, not key 7 as supplied to it from "b" as obtained from "branches".)

[mdjermanovic]

Hm, it works when I remove commonjs.cjs test file. Could we maybe split *.js and *.cjs test runs and run only *.js tests through c8, like in the espree repo?

[brettz9]

Added a commit to split the tests

[mdjermanovic]

Added a commit to split the tests

It works now, thanks!

[brettz9]

Regarding the dist not exporting map, I've made the change, but just to be clear, as per the parity with espree, the Rollup file does build a sourcemap, so it is available for the repo/development at least.

Likewise with default having been carried over.

[mdjermanovic]

LGTM, thanks!