📎 Implement noGlobalDirnameFilename rule

[kibertoad]

Description

__dirname doesn't work in ESM context, but is not easily caught by tests, as e. g. vitest injects __dirname, so it works correctly there, but then fails in production. __filename is also in a similar situation.

Prior art: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-module.md

Also see #4245

[kibertoad]

@ematipico I don't know Rust, so unfortunately can't implement it myself.

[ematipico]

Then why was this task open? Usually, these issues are open after a previous discussion, or by maintainers.

If there wasn't prior discussion, could you please open a discussion instead?

[kibertoad]

@ematipico I've linked previous discussion, see #4245. I believe you thumbs uped it as well :D

There was an ask to open a task for it from one of the collaborators.

[unvalley]

We have the noCommonJs rule, but it only applies to imports and exports.

If we want to add the prefer-modules rule, I think we should break it down into a few specific rules.

• Disallows 'use strict' directive.
  • noRedundantUseStrict done this
• Disallows “Global Return”.
  • noGlobalReturn?
• Disallows the global variables `__dirname and __filename.
  • noGlobalDirnameFilename this issue
• Disallows require(…).
  • noCommonJs done this
• Disallows exports and module.exports.
  • noCommonJs done this

[unvalley]

noGlobalLegacyPath might be better, because we have already noGlobalXXX rules.

[dyc3]

IMO from a user's perspective "noGlobalLegacyPath" reads like it's banning something related to paths, not global variables

[unvalley]

noGlobalDirnameAndFilename? It's not cool but expressing itself directly and easy to search.

[Conaclos]

Disallows 'use strict' directive.

Is this not covered by noRedundantUseStrict?

Disallows “Global Return”.

It is already a syntax error in module. Do I miss something?

Disallows the global variables `__dirname and __filename.

I wonder, is there reason to create a dedicated rule or should we ad this to noCommonJs?

noGlobalDirnameAndFilename?

noGlobalDirnameFilename should be fine.

[dyc3]

should we add this to noCommonJs

From a quick search, it looks like these globals are only valid in commonjs environments. So I think yeah, it would make sense to put it in the same rule. However, things against this that I can think of is:

• that we could be taking some granularity away from users.
• vitest injects these variables when it runs, so these can be available in esm looking environments, IIRC.

For those reasons I think it would make sense to keep this as a separate rule, or have options for the noCommonJs rule to enable/disable these different aspects of the rule. Adding rule options would be weird IMO, we generally tend to add rules and minimize the scope of those rules as much as possible.

[unvalley]

Is this not covered by noRedundantUseStrict?

It's covered! I missed it.

[Conaclos]

For those reasons I think it would make sense to keep this as a separate rule, or have options for the noCommonJs rule to enable/disable these different aspects of the rule. Adding rule options would be weird IMO, we generally tend to add rules and minimize the scope of those rules as much as possible.

You made a good point. Let's keep the rule separated. We should certainly add a pointer to the new rule in noCommonJs.