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

Breaking: Switch to ESM (fixes #35) #39

Merged
merged 20 commits into from Aug 4, 2021
Merged

Breaking: Switch to ESM (fixes #35) #39

merged 20 commits into from Aug 4, 2021

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Jun 25, 2021

Fixes #35.

  • Breaking: Switch to ESM; fixes Convert package to ESM #35
  • Build: Align Rollup with eslint-scope
  • Chore: Update devDeps.
  • Chore: Fix a YAML parsing test (was failing in main)

Note that I converted CommonJS files to use the cjs extension as per type: module.

I can align this to any file changes on #37.

@eslint-github-bot
Copy link

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

1 similar comment
@eslint-github-bot
Copy link

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 brettz9 changed the title ESM Breaking: Switch to ESM Jun 25, 2021
@brettz9 brettz9 changed the title Breaking: Switch to ESM Breaking: Switch to ESM (fixes #35) Jun 25, 2021
Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

It make senses to covert conf/* to esm

.eslintrc.cjs Outdated Show resolved Hide resolved
lib/shared/relative-module-resolver.js Outdated Show resolved Hide resolved
@aladdin-add aladdin-add marked this pull request as draft July 3, 2021 08:15
@brettz9
Copy link
Contributor Author

brettz9 commented Jul 3, 2021

It make senses to covert conf/* to esm

It will force a change to the API; in particular in ./lib/config-array-factory.js, loadConfigFile is called at run-time by loadInDirectory and various other methods through _loadConfigData. loadConfigFile in turn calls loadJSConfigFile and then importFresh with no Promises/async being in use.

See discussion beginning at #35 (comment)

@aladdin-add aladdin-add marked this pull request as ready for review July 3, 2021 09:05
@aladdin-add aladdin-add self-requested a review July 3, 2021 09:12
@nzakas
Copy link
Member

nzakas commented Jul 10, 2021

We for sure don’t want to change the API. The API overall is frozen, so anything we can do to avoid changes is important.

@brettz9
Copy link
Contributor Author

brettz9 commented Jul 17, 2021

I think this is ready for further review. It keeps the old API by using CommonJS for the conf files.

@nzakas
Copy link
Member

nzakas commented Jul 28, 2021

Thanks. This is on my todo list to review!

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

Prepared eslint/eslint#14865, which currently uses this branch. That should be a good test for these changes.

Comment on lines +30 to +46
const Legacy = {
ConfigArray,
createConfigArrayFactoryContext,
CascadingConfigArrayFactory,
ConfigArrayFactory,
ConfigDependency,
ExtractedConfig,
IgnorePattern,
OverrideTester,
getUsedExtractedConfigs,

// shared
ConfigOps,
ConfigValidator,
ModuleResolver,
naming
};
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 we should add environments here since ESLint needs that:

https://github.com/eslint/eslint/blob/e31f49206f94e2b3977ec37892d4b87ab1e46872/lib/linter/linter.js#L19

The exported name could be BuiltInEnvironments?

Copy link
Member

Choose a reason for hiding this comment

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

I'll add that.

lib/shared/config-validator.js Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
tests/_utils/index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
brettz9 and others added 4 commits August 2, 2021 08:01
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

This looks really good @brettz9! Thanks so much for all of the hard work.

Comment on lines +30 to +46
const Legacy = {
ConfigArray,
createConfigArrayFactoryContext,
CascadingConfigArrayFactory,
ConfigArrayFactory,
ConfigDependency,
ExtractedConfig,
IgnorePattern,
OverrideTester,
getUsedExtractedConfigs,

// shared
ConfigOps,
ConfigValidator,
ModuleResolver,
naming
};
Copy link
Member

Choose a reason for hiding this comment

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

I'll add that.

@nzakas nzakas merged commit bdce01a into eslint:main Aug 4, 2021
@nzakas nzakas mentioned this pull request Aug 4, 2021
@brettz9 brettz9 deleted the esm branch August 4, 2021 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert package to ESM
4 participants