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

Allow mapping of expected extensions in import/extension rule #2033

Closed
wants to merge 1 commit into from

Conversation

pwhissell
Copy link

Because all of the following requirements are true:

Then if someone wants to use Typescript to gereate nodejs apps that use "top-level await", it makes sens to start usiung nodejs's
newly supported ESM. (this is also probably going to apply to more and more module functionalities). To do so, while developping, people have to make sure that all relative imports comply with nodejs's requirement to specify the trailing ".js" file extension.

Typescript has no functionality to enforce ".js" file extensions. Because tslint was deprecated in favor of eslint, I feel
this eslint plugin would make a lot of people happy in filling out the gap.

Note this is currently a draft which more work

  • implement feature/fix bug
  • write tests
  • update docs
  • make a note in change log

fixes #2030

@pwhissell pwhissell marked this pull request as draft April 25, 2021 00:58
@coveralls
Copy link

Coverage Status

Coverage increased (+1.7%) to 71.201% when pulling e4bff8e on pwhissell:master into e871a9a on benmosher:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+1.7%) to 71.201% when pulling e4bff8e on pwhissell:master into e871a9a on benmosher:master.

@coveralls
Copy link

coveralls commented Apr 25, 2021

Coverage Status

Coverage increased (+1.9%) to 71.446% when pulling e4bff8e on pwhissell:master into e871a9a on benmosher:master.

@ljharb
Copy link
Member

ljharb commented Apr 25, 2021

I'm still pretty confused here.

If you want to require an extension, configure the rule to do so.

If you want to only require an extension in .ts files, use overrides, set files to **/*.ts, and configure the rule to do so there.

What does this PR allow you to do that you can't already do with an eslint config?

@pwhissell
Copy link
Author

I'm new to eslint so I wouldn't be suprised that I misunderstand something, but here is how I understand "overrides":

"overrides": [
    {
      "files": ["**/*.ts"],
      "rules": {
        "import/extensions": "#Some Config That Would Force All (or relative) Imports To Have .js extension#"
      }
    }
  ]

I havn't found any configuration that would enforce a specific (".js" in my case) extension for all imports

@pwhissell
Copy link
Author

pwhissell commented Apr 25, 2021

Given the following files:

tsconfig.json
file1.ts
file2.ts

if the content of file1.ts is:

import {something} from "./file2"
import {other} from "./file2.js"

the typescript module resolver resolves both "./file2" and "file2.js" to the following file: "./file2.ts" which is right because the resolution happens before transpilation.
When using the rule: import/extensions, the expected extension is chosen from the file name resolved by the module resolver. In this case because the resolved module is defined in the "./file.ts" file, the plugin expects the extension to be ".ts".
The linting error disapears if you right the import statement like so:
import {other} from "./file2.ts"
This import statement is invalid and flagged by Typescript as such.

This PR allows the developper to configure the "import/extensions" in a way that would rather make the linter expect ".js" extension in all import statements even if the resolved module is a file with another extension.
In my example, the linter would now expect and accept nothing other than:

import {other} from "./file2.js"

@ljharb
Copy link
Member

ljharb commented Apr 25, 2021

You’re trying to force an extension appear in code that doesn’t actually exist on the filesystem?

@pwhissell
Copy link
Author

pwhissell commented Apr 25, 2021

im trying to force an extension on an import of a module that does not yes exist. It is really awkward but typescript ensures you that it will. Otherwise typescript raises an exception if it cant make that garantee

@pwhissell
Copy link
Author

typescript should definately implement such a functionality but this plugin is 80 lines away from doing it

@pwhissell
Copy link
Author

pwhissell commented Apr 25, 2021

its a non-intrusive solution that would please any nodejs+typescript enthousiasts
even more so now that tslint is deprecated in favor of eslint.

perhaps the proposed solution could be extended to other transpilers or contexts in its flexibility. Or should I change the implementation to focus on ts transpilation?

@pwhissell
Copy link
Author

pwhissell commented Apr 25, 2021

im linting the ts file and not the transpiled js file

@ljharb
Copy link
Member

ljharb commented Apr 25, 2021

The proper solution here would be to implement this as an option in the typescript resolver, not in this plugin itself.

@pwhissell
Copy link
Author

I think your opinion is very defendable, but I would argue that the resolver is correct in that it resolves the module to the right file. Concerning separation of concerns, I also argue that enforcing a specific extension fits very well with this module's responsibility.
If this were an invasive solution, I wouldn't even have considered the option of a PR.

@ljharb
Copy link
Member

ljharb commented Apr 26, 2021

It’s that the extension doesn’t already exist that’s the problem. This plugin is only attempting to combine node’s CJS resolution algorithm with a style preference.

@pwhissell
Copy link
Author

Hahah now I guess I'm confused :S

This plugin is only attempting to combine node’s CJS resolution algorithm
When reading the readme from the root of this repo, I found that typescript the resolver is supported. I also appreciated got inspired by existing unit tests, when specifying validating my functionality for all supported typescript versions.

I may have missinterpreted the readme when reading that this repo was "trying to support linting of ES2015+ (ES6+) import/export syntax, and prevent any issues with misspelling of file paths and import names."

I also think that linters are not ownly good for validating stylistic errors, but also (and probably more importantly) programmatic errors.

@pwhissell
Copy link
Author

I think the un-introssiveness of the implementation makes this at least worth a second and third opinion
@benmosher, @jfmengels, @rfermann what do you guys think

@ljharb
Copy link
Member

ljharb commented Apr 26, 2021

It was only very recently that node changed the plan and decided not to auto-add extensions for ESM. As such, either we enforce what babel/ts does (no extensions, primarily) or what node does (.mjs or, sometimes, .js, but only when the files exist).

It seems like you’re trying to work around typescript’s poor integration with node’s native ESM support, rather than accepting that the two shouldn’t be used together yet.

@pwhissell
Copy link
Author

Typescript should indeed improve esm support, but they wont, because they point to here saying that linting should be the solution.

Given the popularity of the MEAN stack I think having typescript accross the entire stack is goingg to be a trend for the future. Before this can happen, I think features like tgese need to be implemented.

In the context where the imported module exists (in a version not yet transpiled),
I honestly don't grasp the importance of the idea that the imported file needs to exist before linting

@pwhissell
Copy link
Author

It seems like you’re trying to work around typescript’s poor integration with node’s native ESM support, rather than accepting that the two shouldn’t be used together yet.

I wouldn't call it working around, I woulf call it "improve"

As such, either we enforce what babel/ts does (no extensions, primarily)
Typescript does support imports with extensions:
given a file called "y.ts"
import x from y.js would

  • at transpile time resolve to the y.ts
  • at runtime time resolve to the y.js (which is the transpiled y.ts)

@ljharb
Copy link
Member

ljharb commented Apr 26, 2021

And prior to transpile time, would explicitly point to nothing - which is when the linter runs. It would likely work better to transpile your TS with babel, and transform the specifiers to add .js in the build process, and continue to omit them in your source code - then you don’t need to do somersaults in your linter config to support this case.

@pwhissell
Copy link
Author

prior to transpile time, it does not point to nothing. When you use a typescript resolver, it points to the ts file

@pwhissell
Copy link
Author

You suggest I don't transpile my ts files with Typescrtipt but rather do it with Babel?

@ljharb
Copy link
Member

ljharb commented Apr 26, 2021

Always, yes. Babel is much better at transpiling TS than tsc is.

When you use a typescript resolver, it points to the ts file

if that is true, then i'm not sure why any change is needed - the .js extension should Just Work already.

@pwhissell
Copy link
Author

pwhissell commented Apr 26, 2021

Exactly the js extension does work perfectly
typescript supports both "no extension" and ".js" extension.
The problem arizes when you want your project to always use one or the other but not both.
This is when eslint-plugin-import comes in.

Currently the plugin takes for granted that the file's name in which the module is defined has the same extension as the module itself.
In the case of typescript modules, the extension of the file that defines the module (.ts) doesn't correspond to the the extension of the module itself (.js)
this applies for any transpilers that require different extensions for pre-transpiled files (if there are any out there)

Always, yes. Babel is much better at transpiling TS than tsc is.

I think that this is a valid opinion which I share in some regards (it seems more and more so as time moves on). However until typed-objects become more stable I will be sticking to typescript (I don't think I will be the sole developper to do so out there)

@pwhissell
Copy link
Author

pwhissell commented Apr 28, 2021

transpiling with babel doesn't solve the problem. The workaround is to rename typescript files so that the extension is ".js" rather than ".ts" which is even more awkward

there really is no way to get this to pass is there? I'm not tied to the implementation at all, but allowing the linter to expect module extensions other than the extension of the file it is defined in seems perfectly aligned with the scope & goal of this repo.

@pwhissell pwhissell marked this pull request as ready for review April 30, 2021 16:17
@pwhissell
Copy link
Author

this would fix microsoft/TypeScript#16577

@pwhissell
Copy link
Author

Just found this . You can basically configure how nodejs resolves esm modules. running nodejs with the following flag solves the initial problem behind this PR "node --experimental-specifier-resolution=node"

@evelynhathaway
Copy link

I am a little hesitant to add my voice in this thread with the hostility in it, but I am looking for a similar solution. I am sorry this has become a tough issue for those involved.

If this pull request is stuck between a rock and a hard place, is there a possibility of allowing resolver plugins to implement this mapping while we are in this uncanny valley of TypeScript + Node.js ESM? I imagine the rule primarily being used for TypeScript and gradually being faded out depending on whether the module specifier resolution aligns more with node, or TypeScript implements a solution for full import paths besides manually adding them in the TypeScript code with .js extensions.

From my testing, eslint-import-resolver-typescript supports resolving imports using .js extensions in TypeScript imports, even when they don't exist on the file system. I can imagine that plugin wanting to add support for the extension rule as well.

@ljharb
Copy link
Member

ljharb commented May 21, 2021

@evelynhathaway that's an interesting idea - I'm not sure yet how we'd rework things so that was done cleanly, but allowing a custom resolver to do this stuff is a lot safer/simpler than building it into the core plugin.

@ljharb ljharb marked this pull request as draft August 8, 2021 05:47
@pwhissell pwhissell closed this Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Allow mapping of expected extensions in import/extension rule
4 participants