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

fix!: mixed-esm-and-cjs should only add the deviating type aside from base #300

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

voxpelli
Copy link
Member

This avoids specifying **/*.js and makes the base apply to all files that the main config at large is matching, with only deviating type added for either **/.mjs or **/*.cjs

This ensures that the rules still match eg. *.ts, *.jsx etc if the main config does

Copy link

@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.

Can you explain a bit what problem you encountered?

I think it's currently working as expected - type: module means *.js is an esm, but
we cannot tell *.ts is or not.
and it's a breaking change. :)

@aladdin-add aladdin-add changed the title fix: mixed-esm-and-cjs should only add the deviating type aside from base fix!: mixed-esm-and-cjs should only add the deviating type aside from base Jun 17, 2024
@voxpelli
Copy link
Member Author

My first problem was that I used flat/recommended, that made cjs, mjs, js etc all require script or module syntax.

I then opted for flat/mixed-esm-and-cjs to ensure that cjs and mjs functioned correctly, but it then added three configs instead of just the one that differed from what flat/recommended gave me + it limited the base one to only be for *.js files instead as for all non-cjs/mjs as I got with flat/recommended

Maybe I'm thinking wrong, but my impression was that flat/mixed-esm-and-cjs was a flat/recommended that respects cjs / mjs, is there another intention with it?

Maybe we should add a new flat/recommended-mixed or something?

@scagood
Copy link

scagood commented Jun 18, 2024

I am kinda in favour of this change too 🤔

When I prepare my config, I do a fairly similar thing, but I test which "type" is in use, then load the rules accordingly:
image


So:
if "type": "commonjs" | undefined then:

files glob
commonFiles [ '**/*.js', '**/*.cjs' ]
moduleFiles [ '**/*.mjs' ]

if "type": "module" then:

files glob
commonFiles [ '**/*.cjs' ]
moduleFiles [ '**/*.js', '**/*.mjs' ]

@aladdin-add
Copy link

So if I understand you correctly, you want to use this plugin in a ts project?

in the new config, by default, eslint only lints files that match the patterns **/.js, **/.cjs, and **/*.mjs: https://eslint.org/docs/latest/use/configure/configuration-files#specifying-files-and-ignores.

to lint ts, you will need to specify files, something like:

import pluginNode from "eslint-plugin-n";

export default [
...pluginNode.configs["flat/mixed-esm-and-cjs"],
{files: ["**/*.ts"], pluginNode.configs["flat/recommended-module"]}, // if you are writing esm in ts files
]

@scagood
Copy link

scagood commented Jun 19, 2024

no no, sorry do ignore my typescriptFiles variable, this is only about .js, .mjs, and .cjs.


This is just about only doing no-missing-import on esm files, and only doing no-missing-require on cjs files.

We know that we don't need to support some rules on some files, this just makes that specific.

@aladdin-add
Copy link

@scagood it's still not clear to me why the config does not work for you. it seems working as you expected:

{ files: ["**/*.js"], ...recommendedConfig.flat }, // => it's depended on the pkg.type
{ files: ["**/*.mjs"], ...esmConfig.flat },  // recommended-module
{ files: ["**/*.cjs"], ...cjsConfig.flat }, // recommended-script

or can you create an example to illustrate?

@scagood
Copy link

scagood commented Jun 20, 2024

no, you're right!

@voxpelli
Copy link
Member Author

Two things:


Firstly, it duplicates an entire ruleset, since this:

{ files: ["**/*.js"], ...recommendedConfig.flat }, // => it's depended on the pkg.type
{ files: ["**/*.mjs"], ...esmConfig.flat },  // recommended-module
{ files: ["**/*.cjs"], ...cjsConfig.flat }, // recommended-script

Skärmavbild 2024-06-20 kl  18 57 45

Is equivalent to this:

{
  files: ["**/*.js", recommendedConfig.isModule ? "**/*.mjs" : "**/*cjs"],
  ...recommendedConfig.flat
},
recommendedConfig.isModule
  ? { files: ["**/*.cjs"], ...cjsConfig.flat }
  : { files: ["**/*.mjs"], ...esmConfig.flat },

Skärmavbild 2024-06-20 kl  18 57 03

(Screenshots from the config inspector)


Secondly, the docs also say:

config objects that don’t specify files or ignores apply to all files that have been matched by any other configuration object

If I modify the above to become this:

{ files: ['*.jsx'] },
recommendedConfig.flat, // has no files-key
{ files: ["**/*.js"], ...recommendedConfig.flat }, // => it's depended on the pkg.type
{ files: ["**/*.mjs"], ...esmConfig.flat },  // recommended-module
{ files: ["**/*.cjs"], ...cjsConfig.flat }, // recommended-script

Then for a .js file the inspector will show:

Skärmavbild 2024-06-20 kl  19 11 55

The yellow border means that the config directly included the file, no yellow border the config is because its a general config.

For a .jsx file this shows:

Skärmavbild 2024-06-20 kl  19 12 12

The plain flat/recommended is applied to the .jsx file but none of the flat/mixed-esm-and-cjs ones.

I think this is the way to set up flat configs:

[
  { files: ['*.jsx'] },
  ...pluginNode.configs["flat/mixed-esm-and-cjs"],
  unicornPlugin.configs['flat/recommended'],
  securityPlugin.configs.recommended,
]

Not this:

[
  ...pluginNode.configs["flat/mixed-esm-and-cjs"],
  unicornPlugin.configs['flat/recommended'],
  securityPlugin.configs.recommended,
  { files: ["**/*.jsx"], ...pluginNode.configs["flat/recommended-module"] },
  { files: ["**/*.jsx"], ...unicornPlugin.configs['flat/recommended'] },
  { files: ["**/*.jsx"], ...securityPlugin.configs.recommended },
]

@aladdin-add
Copy link

well, I got the point, so we are on the same page now. :)

I'm 100% agree to the 1st point.

I think this is the way to set up flat configs...Not this:...

I'm not fully convinced about the second point. The new config format offers users flexibility in organizing their configurations, and I don't want to force a specific approach on them.

Regarding the exported config:

recommended-module and recommended-script serve as the base configurations, which users can combine as needed.
recommended and mixed-esm-and-cjs are additional configurations exported due to their common usage in specific scenarios. The former is recommended if you have advanced needs.

@voxpelli
Copy link
Member Author

voxpelli commented Aug 5, 2024

The new config format offers users flexibility in organizing their configurations, and I don't want to force a specific approach on them.

It's the current approach that are forcing a specific approach, by actively limiting itself to specific file extensions rather than behaving like ESLint itself does and only specify file extensions for the exceptions from the default

@aladdin-add
Copy link

aladdin-add commented Aug 6, 2024

I'm a bit hesitant about this change: on the one hand it facilitates the use of scenarios like the ones you propose; on the other hand it could also accidently lead to wrong configurations being applied to these files: for example, cjs configs applied to ts written with esm, and even non-js files could apply configs specific to js.

As eslint is introducing a new concept language to allowing linting non-js, so I'm lean to letting users explicitly specify non-js configs. thoughts? @scagood

@voxpelli
Copy link
Member Author

voxpelli commented Aug 6, 2024

This is the default config that ESLint itself prepends to all configs: https://github.com/eslint/eslint/blob/21d3766c3f4efd981d3cc294c2c82c8014815e6e/lib/config/default-config.js#L66-L69

I think mimicking what they do would be desirable

@scagood
Copy link

scagood commented Aug 6, 2024

I am starting to lean towards just removing all parser/langauge options from the published configs.

The one slight exception here (that I am tossing up in my head) are the global variables and node scope.

That being said, the globals that we currently support (lib/configs/recommended-module.js:16) are a far cry from what node supports (lib/unsupported-features/node-globals.js:30)


I think we may be going about this the wrong way. I am not sure we should have different rules for import/require.

eg: no-extraneous-require and no-extraneous-import probably should just be no-extraneous (or a better name)

Is there something I am missing in regards to why we dont do this?


In the longer term it may be better to simply merge the seperated rules together and publish one recomended config, (and one legacy config)

In the shorter term I think this is a fine approach given what we have.

@voxpelli
Copy link
Member Author

voxpelli commented Aug 6, 2024

eg: no-extraneous-require and no-extraneous-import probably should just be no-extraneous (or a better name)

Yes, if this would be possible, then agree

I am starting to lean towards just removing all parser/langauge options from the published configs.

Agree, especially since the only difference between the two is only three things:

Source type:

<     "sourceType": "commonjs",
---
>     "sourceType": "module",

Globals:

6,7c6,7
<       "__dirname": "readonly",
<       "__filename": "readonly",
---
>       "__dirname": "off",
>       "__filename": "off",
30c30
<       "exports": "writable",
---
>       "exports": "off",
40c40
<       "module": "readonly",
---
>       "module": "off",
60c60
<       "require": "readonly",
---
>       "require": "off",

n/no-unsupported-features/es-syntax has "ignores": ["modules"] in the module config:

157c157,159
<         "ignores": []
---
>         "ignores": [
>           "modules"
>         ]

@voxpelli
Copy link
Member Author

voxpelli commented Aug 6, 2024

Digging further into this:

Out of those globals that are defined: The exports, module, require are already being defined by the sourceType, only the __dirname and __filename isn't, see this ESLint playground

@voxpelli
Copy link
Member Author

voxpelli commented Aug 6, 2024

Took some ideas from neostandard/neostandard#130 as well and made an update that exposes a new recommended-env and which changes mixed-esm-and-cjs to simply be that combined with:

    {
        name: "node/recommended",
        rules: commonRules,
    },

Also adds a new name for mixed-esm-and-cjs that's recommended-mixed.

Unfortunately its very breaking to replace recommended with a config like this, as it changes from a single FlatConfig to an array, FlatConfig[], which makes inclusion of it change from [ recommended ] to [ ...recommended ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants