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

Bug: [flat config] .mjs extension for config file #16580

Closed
1 task done
dburles opened this issue Nov 24, 2022 · 31 comments · Fixed by #17301
Closed
1 task done

Bug: [flat config] .mjs extension for config file #16580

dburles opened this issue Nov 24, 2022 · 31 comments · Fixed by #17301
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion documentation Relates to ESLint's documentation
Projects

Comments

@dburles
Copy link

dburles commented Nov 24, 2022

Environment

Environment Info:

Node version: v19.1.0
npm version: v8.19.3
Local ESLint version: v8.28.0 (Currently used)
Global ESLint version: Not found
Operating System: darwin 22.1.0

What parser are you using?

Default (Espree)

What did you do?

My npm package is commonjs, with .js extensions, alongside ESM with .mjs extensions for running tests. Running npx eslint . results in Node failing to parse the configuration file, as it assumes it to be commonjs.

What did you expect to happen?

The config file should only support a .mjs extension (eslint.config.mjs) to avoid this problem since Node will always execute it as ESM regardless of the type field defined in package.json.

What actually happened?

(Shortened)

Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
...
eslint.config.js:1
export default [
^^^^^^

SyntaxError: Unexpected token 'export'

Participation

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

Additional comments

No response

@dburles dburles added bug ESLint is working incorrectly repro:needed labels Nov 24, 2022
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Nov 24, 2022
@dburles dburles changed the title Bug: [flat config] support .mjs extension for config file Bug: [flat config] .mjs extension for config file Nov 24, 2022
@mdjermanovic mdjermanovic moved this from Needs Triage to Triaging in Triage Nov 24, 2022
@mdjermanovic
Copy link
Member

Hi @dburles, thanks for the issue!

This is not a bug. ESLint looks for an eslint.config.js file and loads it using import(). In commonjs projects, eslint.config.js should be CJS module. In type: "module" projects, eslint.config.js should be ESM module.

The config file should only support a .mjs extension (eslint.config.mjs) to avoid this problem since Node will always execute it as ESM regardless of the type field defined in package.json.

We want to support both CJS and ESM formats for eslint config files, so supporting only .mjs extension is not an option because in that case config files would have to be ESM. We could only consider supporting multiple extensions - eslint.config.js, eslint.config.mjs, and then probably eslint.config.cjs as well - if there are any benefits of having ESM configs in commonjs projects.

Since this is a bug report, I'm not sure if you're asking for ESLint to support ESM configs in commons js projects, or it wasn't clear that in commonjs projects eslint.config.js should be CJS.

@43081j
Copy link
Contributor

43081j commented Nov 24, 2022

fwiw we actually write our source as modules (typescript) and our build target is commonjs.

because of this, we'd naturally also want our eslint config to be an es module (since the rest of our sources are technically).

i think for that reason, it'd be a good idea to detect the multiple extensions and leave the choice up to the consumer here.

@Gstar123456

This comment was marked as spam.

@Gstar123456

This comment was marked as spam.

@dburles
Copy link
Author

dburles commented Nov 24, 2022

Since this is a bug report, I'm not sure if you're asking for ESLint to support ESM configs in commons js projects, or it wasn't clear that in commonjs projects eslint.config.js should be CJS.

Thanks @mdjermanovic, a bit of both, it wasn't super clear that the config should be in the same format as the project I assumed the new config format was ESM only, since that's all I saw in the documentation, with no mention of commonjs.

We could only consider supporting multiple extensions - eslint.config.js, eslint.config.mjs, and then probably eslint.config.cjs as well - if there are any benefits of having ESM configs in commonjs projects.

I think the best solution for now, if commonjs configs are to be supported, is for ESlint config to follow Node with how it handles file extensions. As @43081j mentioned, supporting .cjs and .mjs (.js depends on the package.json type) would allow the choice of format to be up to the user.

There are plenty of benefits to ESM only config regardless of the project type, perhaps it's something that will be considered along with the potential ESlint rewrite (See "Complete rewrite of ESLint" discussion #16557).

@mdjermanovic
Copy link
Member

it wasn't super clear that the config should be in the same format as the project I assumed the new config format was ESM only, since that's all I saw in the documentation, with no mention of commonjs.

I think the documentation should be updated to clarify this.

As @43081j mentioned, supporting .cjs and .mjs (.js depends on the package.json type) would allow the choice of format to be up to the user.

Makes sense to me. Config file search could first check if eslint.config.js exists in the cwd, then eslint.config.mjs, then eslint.config.cjs, then go up one directory and repeat.

@nzakas what do you think?

@mdjermanovic mdjermanovic moved this from Triaging to Feedback Needed in Triage Nov 25, 2022
@mdjermanovic mdjermanovic added enhancement This change enhances an existing feature of ESLint documentation Relates to ESLint's documentation core Relates to ESLint's core APIs and features and removed bug ESLint is working incorrectly repro:needed documentation Relates to ESLint's documentation labels Nov 25, 2022
@nzakas
Copy link
Member

nzakas commented Nov 25, 2022

I agree with updating the documentation. Because most JavaScript is now being written ESM-first, it makes sense that the bulk of the docs be written with that in mind. Also, because this is still an experimental feature, the docs are still WIP as well.

I disagree with supporting other extensions. The intent is to have just one configuration file to avoid needing to check for multiple different files, which is what eslintrc does. Adding in support for .mjs and .cjs just adds complexity with very little tangible value. This was the mistake we made with eslintrc: trying to accommodate everyone’s preferences for config files, and I don’t want to repeat that.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Nov 26, 2022

Most JavaScript is absolutely being authored in ESM, but the vast majority of that is transpiled to CJS, since native ESM has few advantages and many disadvantages over CJS output.

I do, however, agree that eslint configs should be able to have the cjs or mjs extensions, because that’s what node defines as the expected working extensions.

@dburles
Copy link
Author

dburles commented Nov 26, 2022

I agree with updating the documentation. Because most JavaScript is now being written ESM-first, it makes sense that the bulk of the docs be written with that in mind. Also, because this is still an experimental feature, the docs are still WIP as well.

That makes sense, the docs just need to make it perfectly clear that the configuration file format follows your project.

I disagree with supporting other extensions. The intent is to have just one configuration file to avoid needing to check for multiple different files, which is what eslintrc does. Adding in support for .mjs and .cjs just adds complexity with very little tangible value. This was the mistake we made with eslintrc: trying to accommodate everyone’s preferences for config files, and I don’t want to repeat that.

That's a very valid downside, the advantage as @ljharb also mentioned is that it follows the Node convention which is perhaps a bit less confusing to users, and gives them the ability to decide what they would prefer to use in their projects.

I don't hold a strong position either way, it seems like an ESM only config might be too early, which is one way to address this problem, since ESM can of course still import commonjs. However perhaps that would be an easier transition with the ESM rewrite as it would of course be required.

@nzakas
Copy link
Member

nzakas commented Nov 30, 2022

It is definitely too early to enforce ESM-only configs, which is why I find the current eslint.config.js approach -- just write it in whatever format your whole project is written in.

If anyone wants to take a stab at updating the docs, I'd appreciate it.

@43081j
Copy link
Contributor

43081j commented Nov 30, 2022

It is definitely too early to enforce ESM-only configs, which is why I find the current eslint.config.js approach -- just write it in whatever format your whole project is written in.

If anyone wants to take a stab at updating the docs, I'd appreciate it.

Part of the point is our entire project is written in esm but outputs commonjs on build. My devs would naturally want to write this config the same way as the rest of the codebase: esm. Which is why the mjs/cjs extensions node defines would be useful.

However if you're strongly against that, being very clear in the docs that it follows your package type would be ideal and solve the confusion still.

@jaydenseric
Copy link

If your project does not have a package type field of "module", and your ESM is in .mjs files because .js is in CJS mode, and if you need to use a pure ESM dev dependency within the project ESLint config file, you are screwed. The eslint.config.js file is in CJS mode, meaning you can't require the ESM dev dependency. ESLint must add support for eslint.config.mjs files.

If in the future ESLint decides to support only one config file name and format that any project could use, it would be eslint.config.mjs because that will always work as ESM no matter what the package type field is configured to, and the ESM format is capable of importing from both ESM as well as CJS dev dependencies.

@nzakas
Copy link
Member

nzakas commented Dec 2, 2022

I’m not 100% sure, but I think if you pass -c eslint.config.mjs on the command line it might work as-is.

@shirotech
Copy link

A workaround had to do was create a config, publish to npm, and use a plain json config with "extends", no more trying to wrestle with the errors.

@dburles
Copy link
Author

dburles commented Dec 5, 2022

I’m not 100% sure, but I think if you pass -c eslint.config.mjs on the command line it might work as-is.

Unfortunately no, it attempts to parse it as YAML.

@nzakas
Copy link
Member

nzakas commented Dec 5, 2022

@dburles you'll need to use the environment variable to force the file to be interpreted as flat config.

@dburles
Copy link
Author

dburles commented Dec 5, 2022

@dburles you'll need to use the environment variable to force the file to be interpreted as flat config.

Whoops! that does it, thanks. That's a decent workaround.

@github-actions
Copy link

github-actions bot commented Feb 3, 2023

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 Feb 3, 2023
@mdjermanovic mdjermanovic added documentation Relates to ESLint's documentation accepted There is consensus among the team that this change meets the criteria for inclusion and removed enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features Stale labels Feb 6, 2023
@mdjermanovic
Copy link
Member

In the 2022-12-01 TSC Meeting, we decided not to support .cjs and .mjs config files.

I've opened #16864 to discuss async function configs as a way to address #16580 (comment).

Marking this issue as accepted to update Configuration Files (New) docs with the CJS config format for eslint.config.js files. I think we should add a note and one example.

@43081j
Copy link
Contributor

43081j commented Feb 6, 2023

fwiw the arguments against it are incredibly weak.

  • people won't write CJS for much longer
    • we can only hope this is the case, but clearly this is speculation
    • most likely we'll see CJS projects around for some years yet. many projects haven't yet migrated, many which have plugin systems still require CJS plugins and don't yet support ESM plugins
  • we don't want config explosion again
    • supporting CJS/MJS is far from the spaghetti you had in the old config system. as already pointed out to you, node itself follows this exact convention (.cjs, .mjs to distinguish a module type which isn't the one specified in your package manifest)
    • many widely used tools in the ecosystem already implement this exact same strategy (e.g. rollup)

as specified in the docs:
https://nodejs.org/docs/latest-v14.x/api/packages.html#packages_determining_module_system

so @nzakas you're worried implementing the same strategy as everyone else (incl node itself) will lead to the convoluted mess you had before. that'll only happen if you decide to build your own strategy on top of it, which you 100% shouldn't do (a team has already gone to a lot of effort to create this well defined strategy, just use it as-is).

not to sound harsh but you're now the odd one out (eslint). all the other tools are already following the right strategy or have plans to.

edit:

some quick googling of what already follows this strategy:

  • rollup
  • webpack
  • web-test-runner
  • prettier
  • vite

@nzakas
Copy link
Member

nzakas commented Feb 6, 2023

@43081j thanks for the feedback. I assure you we considered all of the ramifications of this decision.

@43081j
Copy link
Contributor

43081j commented Feb 6, 2023

@nzakas and your reasoning is still that you're afraid the config will explode like before?

or is there some other concern you have?

this is a tried and tested, well documented resolution strategy by node itself. if you implement it as-is, no extra behaviours, there's no chance of reintroducing the complexities your old config system had.

for sake of visibility, it would be kind of you to explain what the concerns are if not the one you originally stated

(also not trying to be overly negative here, just wanting to understand why such a decision would be made. especially in the face of many equally popular tools who already went ahead with it)

@nzakas
Copy link
Member

nzakas commented Feb 7, 2023

@43081j I've already commented in this issue and the transcripts and minute meetings from the TSC meeting are public and free for you to view. It's not a good use of our limited time as maintainers to re-argue and re-discuss every decision that people don't agree with.

You can use the -c option to specify any config file you want, including .cjs and .mjs, and they will behave as you expect.

@dburles
Copy link
Author

dburles commented Feb 7, 2023

If -c is a viable solution, I just think the related section in the docs (and config file resolution) needs to be featured more prominently. Right now for some reason it's the very last item, when it should probably be the first. It could also be updated to clearly formalise this approach to using a .cjs or .mjs config file in a project with the opposite package.json type. I imagine that would avoid the confusion that prompted me to open this issue.

@43081j
Copy link
Contributor

43081j commented Feb 7, 2023

100% agree on that

I was under the impression even with the c cli switch, it wouldn't work quite right. So sorry I seem to have completely misunderstood that part of it

As long as there's some way to specify that eslint explicitly loads a common js module and vice versa (in a package with the opposite type), it seems to be problem solved

@dburles
Copy link
Author

dburles commented Feb 8, 2023

One downside to having to specify the config file is that you'll also need to configure ESLint editor extensions specific to the project, eg. (VSCode) microsoft/vscode-eslint#1518 (comment).

@nzakas
Copy link
Member

nzakas commented Feb 10, 2023

Please keep in mind that the docs are still very early as this feature is considered experimental. We will be revisiting, revising, and expanding the docs once flat config reaches GA.

mizdra added a commit to mizdra/stylelint-happy-css-modules that referenced this issue Mar 26, 2023
@mizdra
Copy link
Sponsor

mizdra commented Mar 26, 2023

FYI: The workaround combining #16580 (comment) and #16580 (comment) has worked well for me!

However, it seems that additional configuration is needed in .vscode/settings.json to allow vscode-eslint to load eslint.config.mjs as well. The workaround that most people need is as follows.

// package.json
{
  "scripts": {
    "lint": "ESLINT_USE_FLAT_CONFIG=true eslint -c eslint.config.mjs ."
  }
}
// .vscode/settings.json
{
  "eslint.experimental.useFlatConfig": true,
  "eslint.options": {
    "overrideConfigFile": "eslint.config.mjs"
  }
}

@jfparadis-appomni
Copy link

The deprecation of .eslint* is a significant step forward.

However, maintaining support for eslint.config.js|cjs|mjs probably generates the least surprise for developers because that's how npm packages are intended to work.

The flexibility to use cjs or mjs is huge when converting repos from one type to another or sharing configurations between repos. It allows for example to tackle tech debt in small steps and move faster.

@nzakas
Copy link
Member

nzakas commented Apr 12, 2023

Thanks, we aren't soliciting additional opinions at this point.

@eslint eslint locked and limited conversation to collaborators Apr 12, 2023
@bmish
Copy link
Sponsor Member

bmish commented Dec 30, 2023

For the record, this has been addressed in:

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 documentation Relates to ESLint's documentation
Projects
Archived in project
Triage
Feedback Needed
Development

Successfully merging a pull request may close this issue.