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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃搸 Implement lint rule to require file extension on relative imports - import/extensions #2674

Closed
minht11 opened this issue May 1, 2024 · 6 comments 路 Fixed by #2725
Closed
Assignees

Comments

@minht11
Copy link
Contributor

minht11 commented May 1, 2024

Description

Implement subset functionality of import/extensions, to require file extension on relative imports. import './entry' -> ./entry.ts
Motivation in #3 (comment)

Naming

useImportExtensions, useConsistenImportExtensions, useRequireRelativeImportFileExtension, noExtensionlessRelativeImport (do we need word relative?) or maybe it should fall under useImportRestrictions?

Questions

  1. What to do with ./foo/index.js files, which can be imported as ./foo?
    a. Maybe they should be banned all together? Likely too drastic at first.
    b. fix ./foo -> ./foo/index.js
    c. do nothing? Inconsistent and likely confusing.

  2. What options to support if any?
    In order to move ecosystem forward I would strongly prefer that only two modes would be available: requiring extensions and off. I can see the value in dissallowing some extensions, but that likely can be done later if users really want it.

@Conaclos
Copy link
Member

Conaclos commented May 1, 2024

For now, a rule is not able to query the filesystem. Do you suggest just emitting an error and always proposing appending the .js extension?

./foo could be ./foo.js or ./foo/index.js, we have no way of knowing because we cannot query the fs.

I like the conciseness of useImportExtensions.

What options to support if any?

I think you are right. We don't need any config.

@minht11
Copy link
Contributor Author

minht11 commented May 1, 2024

I think that we can pick extension, based on what other import extensions exist in the file or file extension itself and in most cases it will be good enough.

Regarding ./index.js given the limitations of not being able to query FS, for now pretending they do not exist, likely is also fine. Rule note can mention caveats about it. In extension requiring world implicit index.js wouldn't exist anyway.

Will try to work on implementation.

@Conaclos
Copy link
Member

Conaclos commented May 1, 2024

or file extension itself and in most cases it will be good enough.

Yeah I thought about that.
Anyway this could be an unsafe code fix. That's fine if it is not correct.

./index.js

There are som cases where it can only be an index such as .., ../, ., ./, and foo/.

@minht11
Copy link
Contributor Author

minht11 commented May 5, 2024

I just realized that many projects use pattern such as import "~/foo" @/foo which resolve to src/. Without tsconfig access we have no way to know. I wonder if we should add option to allow them to be treated as relative?

@ematipico
Copy link
Member

ematipico commented May 5, 2024

The implementation of this rule seems to fit our rule useImportRestrictions, why didn't we consider adding the logic of the proposed rule inside useImportRestrictions? cc @minht11 @Conaclos

@minht11
Copy link
Contributor Author

minht11 commented May 8, 2024

I thought about it briefly, but not much before implementation, however I see few reasons:
Not quite sure overloading it is the best idea, even if implementation is similar explaining this rule and import restrictions separately is far simpler. useImportExtensions one day could be recomended by default while importRestrictions are far harder to sell.

Requiring extensions does not change behavior it is almost stylistic choice, while importRestrictions force you to rearchitect which imports you are allowed to use.

Another point that importRestrictions have been in nursery for 9months if we merge them, can one part become stable while other not? Can single be in nursery while rule as a whole not? Not a big point but still.

That said I do not have very strong preference and I can see it both ways, but it is bit more clear having them separate at least until having extensions on imports is not standard and needs more education.

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 a pull request may close this issue.

3 participants