import-js / eslint-plugin-import

ESLint plugin with rules that help validate proper imports.
MIT License
5.51k stars 1.56k forks source link

when using typescript recommended config it should allow for imports of js equivialent #2111

Open lifeiscontent opened 3 years ago

lifeiscontent commented 3 years ago

based off the conversation here: https://github.com/microsoft/TypeScript/issues/16577

if a .ts or .tsx file imports a .js file it will resolve .ts .tsx or .js but the plugin:import/typescript doesn't seem to have support for this functionality.

the js extension is used to reference files that WILL be compiled to, so they technically don't exist, but this is how the TS team is allowing people to write ESM compatible modules in TypeScript.

It would be nice to have this eslint plugin to work with this out of the box so I can enforce that all of my modules import .js files to make sure I'm not missing anything when exporting my package to the web.

leepowelldev commented 3 years ago

@ljharb yes I think thats accurate. I think the pain point is around point 2, however it's not around the output files - the plugin reports that the input files don't exist (which technically they don't).

ljharb commented 3 years ago

… and if you ran your build process, they would exist, and everyone would be happy?

beamery-tomht commented 3 years ago

No, it is not. It's saying that the browser will ask the webserver for the exact URL in the source code, whether it has an extension or not, and that it's up to the webserver to handle it. It's exceedingly easy - and in fact common - for webservers to do extension lookup, and certainly to do index lookup.

Browsers do not have any idea of what an extension is - the word is meaningless in a browser context.

image

It's quite clearly saying you can't omit the extension. I don't even know why you're so persistent about this? The extension is about valid syntax, not about how the browser is requesting the file. I never once affirmed that the browser had any concept of extension is so I don't know why you keep repeating this.

Regarding import-maps, this seems like an extra burden to push on any consumers of an ESM lib. They're also not supported in all major browsers.

leepowelldev commented 3 years ago

… and if you ran your build process, they would exist, and everyone would be happy?

No, they wouldn't exist. The generated files would have a .js extension, but not on the import statements within the files would remain extensionless.

ljharb commented 3 years ago

I'm sorry the wiki you're quoting is incorrect, but that doesn't change the facts.

In scenario 2, you're using .js in your source.

leepowelldev commented 3 years ago

I’ll be honest, I don’t even remember what was being asked for anymore 😂

If it’s that we can use (and enforce) .js extensions via the import/extensions rule without unresolved errors that’d be great.

ljharb commented 3 years ago

Right - it seems like you can already get that by using .js in your source, and running your build process, at which point the linter will be able to resolve those files.

leepowelldev commented 3 years ago

So we run eslint against the final production output?

it seems like you can already get that by using .js in your source

Regardless, this isn't the case at the moment, import/extensions will still produce errors as illustrated in the previous comments. I think i'm going to tap out at this point - we're just going around in circles.

ljharb commented 3 years ago

No, you run eslint against your source, but the build files being present means they're still able to be resolved.

I'm sorry this feels like it's going in circles; I'm trying to understand why this isn't TypeScript's bug to fix (or the typescript eslint resolver).

JounQin commented 3 years ago

They are already been resolved correctly, the problem here is we want to enforce to use .js extension in .ts source codes.


This is actually already supported at alexgorbatchev/eslint-import-resolver-typescript#56.

Just remove "import/extensions" this setting. This resolver resolves the .ts file correctly, but it's not allowed in your configuration.

@ljharb The problem is there is no way to enforce to use .js extension currently for .ts files.

leepowelldev commented 3 years ago

No, you run eslint against your source, but the build files being present means they're still able to be resolved.

We build to a seperate output location that is not co-located with the source. So sadly this wouldn't work for us.

I'm not sure how else to explain the issue without seeing it in action. Maybe @patrickarlt repo he put together would help illustrate it better.

EDIT: just to re-iterate, this isn't about resolving files, as @JounQin mentioned, this works fine. However, because the resolved file extension ends up (correctly) as .ts, this doesn't match the source file import extension .js and we get the Missing file extension "ts" for "./foo.js" error.

// Resolved path is `foo.ts` - so extension is `.ts`
const extension = path.extname(resolvedPath || importPath).substring(1);

...

// The importPath is `foo.js` - so this condition will always be true
if (!extension || !importPath.endsWith(`.${extension}`)) {
 // error
}
leepowelldev commented 2 years ago

Just looking at the TS 4.5 release notes(https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-5.html), and it looks like (unless I'm interpreting it incorrectly) that the .js extensions will need to be manually added to support the new node12 and nodenext targets.

IMO, these two new targets make getting this issue resolved even more important.

AndrewLeedham commented 2 years ago

@ljharb If I were to add an option say extensionOverride in a PR to support this functionality, is that a feature that would land in eslint-plugin-import. Happy to give it a shot just don't want to start the work if you don't see it making sense in this plugin?

Thinking it would work like this:

'import/extensions': [
  'error',
  {
    js: { mode: 'always', extensionOverride: 'ts' },
  },
]
Haroenv commented 2 years ago

What I did in my project (https://github.com/algolia/instantsearch.js) was:

  1. no extensions inside the project, enforced by this plugin
  2. add extensions using a custom babel plugin: https://github.com/algolia/react-instantsearch/blob/master/scripts/babel/extension-resolver.js
  3. test using node (which is the most strict) that all extensions were added: https://github.com/algolia/react-instantsearch/blob/master/test/module/packages-are-es-modules.mjs

This way all tooling works as before, and I get errors if an extension is mistakenly added or if it didn't resolve correctly.

ljharb commented 2 years ago

@AndrewLeedham i don’t think that would make sense, no.

AndrewLeedham commented 2 years ago

@Haroenv thanks for sharing that, looks like a great approach!

@ljharb Fair enough I will leave it for now then.

manovotny commented 2 years ago

I just arrived on scene here yesterday after adding this plugin to our first project using TypeScript and trying to output ESM. It is disheartening to see postering and personal opinion when people are just trying to make it easy to do the right thing in their codebases, at least according to the TypeScript and ESM specs.

Is there any way to refocus and move past opinions and personal preferences toward a meaningful and actionable path forward, again, at least according to the TypeScript and ESM specs?

ljharb commented 2 years ago

@manovotny "the right thing" is always a personal opinion/preference. There's no TS spec, and the ESM spec does not in any way mention specifiers, so the only things that matter are the behavior of browsers (exact URLs only, extensions doesn't exist as a concept), node (extensions are required for relative paths; "exports" means they don't have to be required for anything imported from a package), and babel/typescript (ESM uses the CJS algorithm; this is what almost all code authored with ESM is currently using).

eturino commented 2 years ago

I am probably adding noise (apologies in advance), but I may have missed the solution to my problem. I have a TS for node codebase, which needs to use ESM now. As such, I was following this:

https://www.typescriptlang.org/docs/handbook/esm-node.html which among other things say:

relative import paths need full extensions (we have to write import "./foo.js" instead of import "./foo")

Currently if I do that I get a linter error with

ESLint: Missing file extension "ts" for "./foo.js"(import/extensions)

now, foo will be a foo.ts file indeed. And in fact, if I try to say import "./foo.ts" I get a TS error

TS2691: An import path cannot end with a '.ts' extension. Consider importing './foo.js' instead.

I could turn off import/extensions for all ts files to avoid the warning, but the thing is that I need a linter rule to let me know which extensions are missing, since the ones that are missing will end up not working at run time.

Error [ERR_MODULE_NOT_FOUND]: Cannot find module 'Somewhere/foo' imported from /Somewhere/index.js

The solution to allow an optional override of extensions config to say to the linter "it's a .ts extension, we're cool if you import .js instead" seems like a good solution, but I saw in your message https://github.com/import-js/eslint-plugin-import/issues/2111#issuecomment-1023290190 @ljharb that it doesn't seem right to you.

Am I missing a solution that will get me warnings of missing extensions but still compile right?

I am happy to give coding the overrides a try btw.

ljharb commented 2 years ago
  1. In general, I suggest using babel to transpile TypeScript; it does a better job than tsc does (tsc works great for typechecking TypeScript, ofc)
  2. TS refuses to rewrite specifiers
  3. rewriting specifiers with a babel plugin is trivial
  4. source code should not include extensions (node's decision to require it is an ergonomics loss, and doesn't need to damage your codebase if you're using a build process)
  5. So, if you want to output node-native ESM for some reason, my suggestion is to use a babel plugin that adds in the extensions for you, and since linting only applies to the actual source code, you can omit extensions there.
tilgovi commented 2 years ago

If it's helpful, here's an example of using babel-plugin-module-resolver to do the rewriting: https://github.com/apache/incubator-annotator/blob/main/babel.config.js#L59

The heuristic there is that all relative paths from a .ts file are expected to resolve to a .ts file, but you can use your own heuristic or check for the existence of the target paths.

You can use the related import resolver to have ESLint and this plugin resolve using these rewrites: https://github.com/apache/incubator-annotator/blob/main/.eslintrc.js#L62

I think @ljharb has solid recommendations here. Until this all settles down more, it's easier to work around the rough edges with Babel in the toolchain and let tsc just do typechecking.

I don't agree with point 4, but I won't offer more on my own opinion there. I think this is the kind of opinion that @manovotny was politely suggesting isn't terribly helpful.

ljharb commented 2 years ago

@tilgovi sure. but if you want your source to include extensions, it should include .ts, and your build should transform that to .mjs.

andersk commented 2 years ago

It’s reasonable that you want to avoid adding TypeScript-specific logic to this JavaScript-centered plugin. What do you think about making the issue become eslint-import-resolver-typescript’s problem, as follows?

Extend the resolver plugin API to let it return two paths, an import path and a physical path. Rules like extensions that examine the filename itself would work against the import path, while rules like no-unresolved that examine the file’s contents would read it through the physical path.

By default the import and physical paths would be the same, but eslint-import-resolver-typescript could be modified to return foo.js for the import path and foo.ts for the physical path, to match tsc’s expectations. (This would be controlled by an eslint-import-resolver-typescript option of course, so that it could still be used in Babel-like configurations.)

Is this a solution that would make everyone happy?

ljharb commented 2 years ago

That definitely seems like an appropriate extension to the API, and would probably solve a lot of issues in TS where the physical path is a d.ts file but the import path is not.

How do you propose extending that in a non-breaking way?

tilgovi commented 2 years ago

Is an extension to the API needed? The current resolve interface returns the physical path, and it's called with the import path. Both paths are known, then.

JounQin commented 2 years ago

@tilgovi Maybe you can draft a PR for it.

tilgovi commented 2 years ago

What need is not being met by the TypeScript resolver today?

JounQin commented 2 years ago

What need is not being met by the TypeScript resolver today?

TS resolver is working as expected, this plugin does handle extensions correctly.

tilgovi commented 2 years ago

I think it's perhaps only the "missing file extension" warnings when using .js to import from the TypeScript build output files. I'll take a look and see what can be done. Maybe import/extensions should be acting on the import path rather than the resolved path.

JounQin commented 2 years ago

import path rather than the resolved path.

I totally agree with this, but it could be a BREAKING change. A new setting can be provided.

tilgovi commented 2 years ago

The extensions rule knows both the import path and the resolved path (if it resolves). It needs to use the resolver to support checking whether the resolved path is a package or not.

Currently, when the path resolves, the extensions rule is getting the expected extension from the resolved path. The error happens because the resolved .ts file doesn't end in the expected .js.

The TypeScript team has made it clear that they don't think TypeScript should transform paths during compilation. They are considering making it possible to reference .ts files in the source, though (microsoft/TypeScript#37582). They're not going to transform .ts into .js in this mode, but expect to use it for compilation targets where the build output is not JavaScript and the runtime isn't Node.js such that the .ts files will actually be present at runtime. Things are even weirder when there are bundlers involved, because there's actually no files imported at runtime.

So we need to be careful not to haphazardly expand the role of the resolver. Let's take a step back and ask what the user expects the extensions error to do. Are they trying to prevent a runtime error? Are they trying to prevent a bundler error? A compilation error? Are they just enforcing a stylistic preference?

tilgovi commented 2 years ago

So we need to be careful not to haphazardly expand the role of the resolver.

For example, just because the resolver might be able to tell us what file should resolve to at runtime doesn't mean that's what the user wants their source to say. That may be what TypeScript users want today for a Node.js runtime target, but it might not be what someone else wants today or tomorrow.

tilgovi commented 2 years ago

If we do want to assume that it's the resolver responsibility to error if a source path is incorrect, we could make the change I was suggesting and use the import path's extension should or should not be present given the options of the extensions rule, as I said above.

As @JounQin said, that'd be a breaking change. And as I said, that'd be enshrining a particular role for the resolver that may or may not be quite the same as its original purpose. It would be saying that the resolver's role is not just to help this plugin find files to check, but to say whether or not the import is correct. That's a bit different than a stylistic check, though. That's saying it's the resolvers responsibility to hard error when the user writes .ts and should have written .js. Instead of resolving the .ts file, it should say no, ./foo.ts does not resolve.

JounQin commented 2 years ago

@tilgovi

The new behavior can be enabled under a new setting flag, then there will be no breaking change.

If you can contribute to draft a PR, that would be great!

tilgovi commented 2 years ago

I'll sleep on it first. I'm not sure how to name the flag or communicate what it does yet. Any suggestions are welcome.

JounQin commented 2 years ago

I'll sleep on it first. I'm not sure how to name the flag or communicate what it does yet. Any suggestions are welcome.

Good night. How about preferImportExtension?

justingrant commented 1 year ago

@JounQin @tilgovi Was there a conclusion from the discussion above about a "desired file extension" for TS relative imports?

BTW, another thing that could be helpful about this option would be if this plugin were extended in the future to support auto-fix of this problem by adding missing extensions... although for non-TS files I'm not sure how valuable that would be.

jordaaash commented 1 year ago

FWIW, I wrote an eslint plugin that requires js extensions, and also fixes them: https://github.com/solana-labs/eslint-plugin-require-extensions

tpluscode commented 1 year ago

FWIW, I wrote an eslint plugin that requires js extensions, and also fixes them: https://github.com/solana-labs/eslint-plugin-require-extensions

Finally, the savior has arrived. Thank you for that

leppaott commented 1 year ago

FWIW, I wrote an eslint plugin that requires js extensions, and also fixes them: https://github.com/solana-labs/eslint-plugin-require-extensions

This works but this plugin's ignorePackages would be needed to be usable. A shame this hasn't been resolved yet.

But recommended rules with 'import/no-unresolved': 'off', indeed work see @patrickarlt 's commend.