feat: add enforceEsmExtensions option to import/extensions rule

[SgtPooki]

This rule can help you use the proper full path to an import, with extension. Supports --fix.

• I've used this rule multiple times now to support migrating from cjs to esm. see https://gist.github.com/SgtPooki/65e189531f4a5366ef4f80825feb2e5f

• It also helps when editors auto-populate an import but don't provide the full path with extension.

note: No tests have been added by me so anyone willing to help add those, please feel free. I won't be able to work on tests for this PR for a bit

You can see this rule in effect (via patch-package) in the commit ipfs/ipfs-webui@5aa8035

[ljharb]

With native ESM in node, only relative paths should use extensions, as you indicated.

Why does this need a separate rule instead of being an option (or a configuration) on import/extensions?

[SgtPooki]

Why does this need a separate rule instead of being an option (or a configuration) on import/extensions?

I guess it doesn't. I can look into moving it over to a configuration on import/extensions

[SgtPooki]

@ljharb updated. lmk if this is reasonable. we could update the rule config to accept an array of replaceable extensions instead of hardcoding them. I will do that later if reasonable.

[ljharb]

maybe instead of hardcoding ESM here (or java-ly capitalizing it "Esm"), this could be an option that takes always or never, and solely applies to relative paths?

Meaning, relativePaths: "always" would be the node native ESM approach (since that and Deno are the only environments where extensions are required for native ESM - browsers don't, and any transpiled ESM shouldn't).

[SgtPooki]

java-ly..

hah.. "ESME xtensions" isn't any better ;)

yea it should only apply to relative paths. I am fairly certain that I plugged it into the existing rule where that is true but let me know if I missed something.

Since we're already talking about extensions an option with extensions in the name is superfluous. I'm fine with 'relativePaths', but also thought about using 'fullySpecified' similar to webpack's resolver naming: https://webpack.js.org/configuration/module/#resolvefullyspecified

[ljharb]

if we're going to make it autofixable, let's make sure it can autofix everything, not just this new option?

it might be nice to have a separate PR that makes the rule autofixable?

[SgtPooki]

What I saw from adding the tests for this option is that all the "valid" usecases where imports are fixed will need updating, so the PR could quickly become very large.

I would much rather open a separate PR for autofixing everything.

[SgtPooki]

to be fair though, many of the failures don't have valid fixes. i.e. import not found... how do we want to handle those? remove the import?

[ljharb]

Suggested change

	const esmExtensions = ['.js', '.ts', '.mjs', '.cjs'];
	esmExtensions.push(...esmExtensions.map((ext) => `/index${ext}`));
	const esmExtensions = [].concat(
	'.js',
	'.mjs',
	'.cjs',
	esmExtensions.map((ext) => `/index${ext}`)
	);

the .ts extension should definitely not be here since it's not ESM.

also, the .js extension isn't always ESM, only when type: module is present, so this might need to be more complex.

[SgtPooki]

Your code isn't valid, but I can remove the 'ts' extension

w.r.t typescript extensions, here's the gotcha: in Typescript packages, you have to import foo from './foo.js' instead of import foo from './foo.ts'. These extensions are the extensions to check to validate that the path is real, but it should still be replacing a valid .ts with .js. Currently, the rule will actually set the import to '.ts' instead of '.js'. Which needs fixed...

For this option to be robust in the current landscape, we should really accept a mapping of expected fileExtensions, and the extension to use in the source. something like:

{
"import/extensions": ["error", "always", { 
    "esm": {
        "extensionOverride": { 
            "ts": "js", 
            "tsx": "jsx"
        }, 
        "extensions": ["js", "ts", "cjs", "mjs"]
    }}]
}

where extensions without overrides are replaced with themselves. What do you think about this.

[SgtPooki]

also, the .js extension isn't always ESM, only when type: module is present, so this might need to be more complex.

I think for this rule, we can assume when someone enables it that they are using ESM for their non cjs files. Would a comment in the doc cover this usecase?

maybe changing the name to 'fullyResolved' would help communicate more accurately what it's fixing

[SgtPooki]

relevant issue: microsoft/TypeScript#49083 (comment)

allowing overrides would let the consumers leave 'ts' replacements as 'ts' (deno migrations) or swapping ts for js (esm migration in typescript project)

[ljharb]

what is invalid about that code? it works perfectly fine.

I understand about the TS-specific quirks but that should come from the TS resolver and not be hardcoded into a rule that might not be running on TS at all.

I do not think we should ever assume that someone is using type:module, which is harmful and should be avoided anyways (altho i believe TS's implementation requires it, not everyone uses TS).

[ljharb]

rofl ok fair enough, good catch. in that case, let's keep it as is, but still without TS.

Suggested change

	const esmExtensions = ['.js', '.ts', '.mjs', '.cjs'];
	esmExtensions.push(...esmExtensions.map((ext) => `/index${ext}`));
	const esmExtensions = ['.js', '.mjs', '.cjs'];
	esmExtensions.push(...esmExtensions.map((ext) => `/index${ext}`));

however we'll still need to handle that .cjs isn't ESM, and .js is only ESM in type:module.

Unless this is "extensions that might exist in ESM files", and then i'm not sure why it wouldn't include wasm and json?

[SgtPooki]

Right, thats why im thinking we should change the rule to "fullyResolved" and then allow users to indicate the extensions they care about, as well as whatever mapping they need.

For my usecase (typescript) i have to check for the existence of both .js and .ts, and replace both with .js

What do you think of that?

[ljharb]

tbh this really seems like a uniquely typescript use case, and it probably should live in the TS eslint plugin (cc @bradzacher).

[bradzacher]

The philosophy of TS is and always has been that you write the import paths that you want at runtime. TS purposely does not transform import paths at all - it will always emit the exact same string after transpilation.

In fact currently it is an error to attempt to import a file using .ts because as far as TS was concerned - there's no "TS runtime", so importing a .ts file makes no sense! EG nodejs can't parse or understand TS, so importing a .ts file will crash things.

Because of this, when they added ESM support they added module resolution logic in the typechecker so that when TS sees ./foo.[cm]?jsx?, it will look for the file ./foo.[cm]?tsx? and will use that.

TS 5.0 includes a flag which makes it so that import './foo.ts' will no longer error for environments where they do understand TS syntax (eg deno or some bundlers). In this instance it will mean that TS will look for that exact file+extension.

To summarise:

• ./foo will cause TS to do the standard node resolution logic, preferring ts extensions
  • eg it'll look for ./foo.ts, then ./foo.tsx, then ./foo.js, then ./foo/index.ts, etc
• ./foo.[cm]?jsx? will cause TS to first look for the corresponding ./foo.[cm]?tsx? file first, then look for the originally specified file.
• ./foo.[cm]?tsx? will cause TS to look for that exact file (with the correct config)

I think this is the priority order of how TS looks up extensions in the ambiguous cases:
https://github.com/microsoft/TypeScript/blob/ddfec78f55bfedd690708c429f803346555e31c4/src/compiler/utilities.ts#L9233-L9234

.d.ts, .d.mts, .d.cts, .mjs, .mts, .cjs, .cts, .ts, .js, .tsx, .jsx, .json

[ljharb]

@bradzacher ok, so i'm not really sure what that means here.

It really seems like it should be an option to the TS resolver, not to any particular rule, but it's pretty complex :-)