eslint / eslint

Find and fix problems in your JavaScript code.
https://eslint.org
MIT License
25.07k stars 4.54k forks source link

Bug: CJS config does not complain about no config object (while ESM config does) #19044

Open abrahamguo opened 1 week ago

abrahamguo commented 1 week ago

Environment

Local ESLint version: 9.13.0

What parser are you using?

Default (Espree)

What did you do?

No "type" declared inside package.json.

Three blank config files:

What did you expect to happen?

ESLint should give the same output no matter which of the three blank config files are used.

What actually happened?

ESLint: 9.13.0

TypeError: Config (unnamed): Unexpected undefined config at user-defined index 0.



### Link to Minimal Reproducible Example

https://github.com/abrahamguo/repro/tree/eslint-no-export

### Participation

- [x] I am willing to submit a pull request for this issue.

### Additional comments

_No response_
mdjermanovic commented 1 week ago

Hi @abrahamguo, thanks for the issue!

ESLint should give the same output no matter which of the three blank config files are used.

What would be the same output in all three cases?

With an empty ESM config file, ESLint reports an error, which is I think correct and useful behavior because it's likely that export default was mistakenly omitted.

CommonJS modules by default export an empty object, so when ESLint gets an empty object we can't know if that was mistakenly omitted export or intentional module.exports = {}.

JoshuaKGoldberg commented 2 days ago

Unexpected undefined config at user-defined index 0.

Thinking out loud: this is a pretty unclear error message for anything other than [undefined /* ... */]. Just by reading it it's not possible to determine that the cause is an empty undefined vs. an [] vs. [undefined].

CommonJS modules by default export an empty object, so when ESLint gets an empty object we can't know if that was mistakenly omitted export or intentional module.exports = {}.

How about a warning when there's any kind of empty value? It feels to me that linting with no configuration at all is a pretty rare case.

Straw man proposal for a few different values:

mdjermanovic commented 1 day ago

While rare, linting with an empty config is a valid use case: it checks whether the code is parsable. @eslint/create-config creates such configs:

$ npx @eslint/create-config
@eslint/create-config: v1.3.1

√ How would you like to use ESLint? · syntax      
√ What type of modules does your project use? · esm
√ Which framework does your project use? · none
√ Does your project use TypeScript? · javascript
√ Where does your code run? · No items were selected
The config that you've selected requires the following dependencies:

eslint
√ Would you like to install them now? · No / Yes
√ Which package manager do you want to use? · npm
☕️Installing...

up to date, audited 86 packages in 698ms

21 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities
Successfully created C:\projects\tmp\tmp\eslint.config.js file.

eslint.config.js:


export default [
];
mdjermanovic commented 1 day ago

Unexpected undefined config at user-defined index 0.

Thinking out loud: this is a pretty unclear error message for anything other than [undefined /* ... */]. Just by reading it it's not possible to determine that the cause is an empty undefined vs. an [] vs. [undefined].

I think we could consider improving the error message for missing export default by throwing an error with a more descriptive message from a place that processes config files, possibly here:

https://github.com/eslint/eslint/blob/dc34f94a2ed25b37ac4aafcabed7bfae582db77e/lib/config/config-loader.js#L202

As for missing module.exports, I think the only way to align the behavior would be to disallow empty {} (but allow empty [] as a way to lint only for syntax errors). That would be a breaking change, though.

@eslint/eslint-team thoughts?

aladdin-add commented 1 day ago

surely [] is a valid config. it can throw an error when:

something like:

const config = (await import(fileURL)).default;

if(!Array.isArray(config)){
  throw new Error("Configuration files should export an array").
}
nzakas commented 12 hours ago

I think @JoshuaKGoldberg's suggestion to always output a warning (console.warn(), not a linting warning) when there's an empty config is a good idea. That wouldn't interfere with people wanting to use an empty config and would at least let them verify that this was intentional. Something like:

Running ESLint with an empty config (filename: /usr/nzakas/eslint.config.js). Please double-check that this is what you want.