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

Change Request: [new config system] Allow async configs #16864

Closed
1 task done
mdjermanovic opened this issue Feb 6, 2023 · 17 comments · Fixed by #17301
Closed
1 task done

Change Request: [new config system] Allow async configs #16864

mdjermanovic opened this issue Feb 6, 2023 · 17 comments · Fixed by #17301
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint

Comments

@mdjermanovic
Copy link
Member

mdjermanovic commented Feb 6, 2023

ESLint version

v8.33.0

What problem do you want to solve?

#16580 (comment)

If eslint.config.js must be a CJS module because the project is "type": "commonjs" (default; i.e., the project does not have "type": "module" in its package.json file), then it isn't possible to use ESM dependencies in eslint.config.js because the only way to load ESM from CJS is by using import(), but that returns a Promise.

What do you think is the correct solution?

Allow async function configs, as proposed in the RFC:

so that ESM dependencies can be used like this:

// eslint.config.js
module.exports = async () => {

    const someDependency = await import("some-esm-dependency");

    return [
        // ... use `someDependency` here
    ];

};

Participation

  • I am willing to submit a pull request for this change.

Additional comments

No response

@mdjermanovic mdjermanovic added enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon core Relates to ESLint's core APIs and features labels Feb 6, 2023
@nzakas
Copy link
Member

nzakas commented Feb 7, 2023

You can already use -c eslint.config.mjs and it will work as expected. It seems like that solves the use case for a CommonJS-only project?

As I've mentioned elsewhere, I just don't think it's a good idea to make concessions for CommonJS at this point in JavaScript's lifecycle. It will be used less and less, as long as we already have a workaround, I don't feel like anything else is needed.

@sam3k
Copy link
Contributor

sam3k commented Feb 10, 2023

Hi @mdjermanovic, based on @nzakas comment, it might make sense to close this issue since there are workarounds. I'll go ahead and close it; but please feel free to reopen if you think otherwise.

@sam3k sam3k closed this as completed Feb 10, 2023
@nzakas nzakas reopened this Feb 10, 2023
@nzakas
Copy link
Member

nzakas commented Feb 10, 2023

@sam3k Please meet @mdjermanovic 😄 He's a member of the ESLint team, so we like to leave these open until the team comes to a resolution. You can notice this on issues by the "Member" tag that appears in the upper-right corner of each comment.

In general, when team members open issues, we can just skip them right to "Ready for Dev Team".

@sam3k
Copy link
Contributor

sam3k commented Feb 11, 2023

@nzakas Ah! My bad. Noted. 😄

@nzakas
Copy link
Member

nzakas commented Feb 14, 2023

@sam3k no problem at all -- I should have mentioned this to you. Feel free to update the triage docs as we discover little glitches in the system like this.

@mdjermanovic
Copy link
Member Author

You can already use -c eslint.config.mjs and it will work as expected. It seems like that solves the use case for a CommonJS-only project?

It works on the command line, but requires additional setup in IDEs and therefore doesn't seem like a convenient workaround.

As I've mentioned elsewhere, I just don't think it's a good idea to make concessions for CommonJS at this point in JavaScript's lifecycle. It will be used less and less, as long as we already have a workaround, I don't feel like anything else is needed.

I think we should provide a way for the ESLint ecosystem to switch to ESM while not dropping support for CommonJS end users. If ESM plugins cannot conveniently be used in CJS projects, that will force plugins (and shareable configs) to stay on CJS or at least provide a CJS export, which means that they cannot use ESM-only dependencies.

@mdjermanovic mdjermanovic removed the triage An ESLint team member will look at this issue soon label Feb 21, 2023
@nzakas
Copy link
Member

nzakas commented Feb 27, 2023

I think we should provide a way for the ESLint ecosystem to switch to ESM while not dropping support for CommonJS end users. If ESM plugins cannot conveniently be used in CJS projects, that will force plugins (and shareable configs) to stay on CJS or at least provide a CJS export, which means that they cannot use ESM-only dependencies.

I understand this line of thinking, I just disagree. Plugins can choose to also export CJS entrypoints if they want compatibility; current CommonJS users can stick with .eslintrc for probably the next year or two.

I just really don't like the complexity tradeoff to support a shrinking number of users. I also think that nudging more folks towards ESM is arguably a net positive for the ecosystem.

At the least, I think we should defer a decision here until we send out our survey and see what sort of data we get back about CJS vs. ESM.

@morganney
Copy link

At the least, I think we should defer a decision here until we send out our survey and see what sort of data we get back about CJS vs. ESM.

Why? You already know CJS will be a shrinking population of the user base. Why not stop all future development supporting CJS to some semver and start all future feature development on a different semver that only supports ESM?

@nzakas
Copy link
Member

nzakas commented Mar 20, 2023

@morganney As you can tell from this thread, there is disagreement on this approach, and so gathering some data will help to make the decision easier.

@github-actions
Copy link

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Apr 21, 2023
@Rec0iL99
Copy link
Member

Not stale. Discussion around the issue is going on.

@Rec0iL99 Rec0iL99 removed the Stale label Apr 25, 2023
@fasttime
Copy link
Member

It is already possible to use asynchronous code (like dynamic imports) in a CommonJS config file by exporting a promise. For example, to amend the sample config in the original post, one could simply wrap the async function in an IIFE, i.e.

// eslint.config.js
module.exports = (async () => {

    const someDependency = await import("some-esm-dependency");

    return [
        // ... use `someDependency` here
    ];

})();

To tell the truth, I've already used this pattern before (repro). Now I realize that this usage was not intended.

@mdjermanovic
Copy link
Member Author

It is already possible to use asynchronous code (like dynamic imports) in a CommonJS config file by exporting a promise. For example, to amend the sample config in the original post, one could simply wrap the async function in an IIFE, i.e.

// eslint.config.js
module.exports = (async () => {

    const someDependency = await import("some-esm-dependency");

    return [
        // ... use `someDependency` here
    ];

})();

To tell the truth, I've already used this pattern before (repro). Now I realize that this usage was not intended.

Interesting, this really works in the current version because loadFlatConfigFile() just returns that promise and it ends up being await-ed before used as a config object. I think this wouldn't work in versions before df77409.

I'm pretty sure this wasn't intended to work, but perhaps we could make it official by adding tests.

@fasttime
Copy link
Member

fasttime commented May 25, 2023

Interesting, this really works in the current version because loadFlatConfigFile() just returns that promise and it ends up being await-ed before used as a config object. I think this wouldn't work in versions before df77409.

This seems to be already working in v8.23.0, hence before that commit.

I'm pretty sure this wasn't intended to work, but perhaps we could make it official by adding tests.

There is clearly a use case here, so it seems reasonable to add unit tests. I'd be also not opposed to documenting the current behavior, rather than keeping it as an Easter egg. We should wait to see what @nzakas thinks.

@nzakas
Copy link
Member

nzakas commented Jun 19, 2023

Nice find @fasttime! Given that this has worked for the past eight months, I'm in favor of documenting it as expected functionality and adding tests to verify. 👍

@simlu

This comment was marked as off-topic.

@mdjermanovic mdjermanovic changed the title Change Request: [new config system] Allow async function configs Change Request: [new config system] Allow async configs Jun 19, 2023
@mdjermanovic
Copy link
Member Author

Looks like we've agreed to officially allow eslint.config.js config files to export a Promise. That solves the problem of using ESM dependencies in CJS config files.

I can work on this.

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jun 19, 2023
@mdjermanovic mdjermanovic self-assigned this Jun 19, 2023
mdjermanovic added a commit that referenced this issue Jun 22, 2023
* feat: allow flat config files to export a Promise

Fixes #16864, #16580

* Update docs/src/use/configure/configuration-files-new.md

Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>

* Update docs/src/use/configure/configuration-files-new.md

Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>

---------

Co-authored-by: Nicholas C. Zakas <nicholas@humanwhocodes.com>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 20, 2023
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants