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

Update import/export specifier sort on rename #57975

Open
6 tasks done
andrewbranch opened this issue Mar 27, 2024 · 7 comments
Open
6 tasks done

Update import/export specifier sort on rename #57975

andrewbranch opened this issue Mar 27, 2024 · 7 comments
Labels
Experience Enhancement Noncontroversial enhancements Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@andrewbranch
Copy link
Member

πŸ” Search Terms

organizeImports import sort rename

βœ… Viability Checklist

⭐ Suggestion

Renaming an exported function that is imported as one of many named imports can make that list of import specifiers become unsorted. When we execute auto-imports, we try to detect how/whether an existing list of named imports is currently sorted and insert the new specifier in the correct spot. We could do something similar when updating an import or export specifier during a Rename command.

πŸ“ƒ Motivating Example

We have an eslint rule that enforces import sort order. I just renamed a function impliedNodeFormatForEmit to getImpliedNodeFormatForEmit, and afterwards I had to go fix the eslint rule failure in 6 different files. It would be nice not to have to do that.

πŸ’» Use Cases

  1. What do you want to use this for? Easier refactoring that doesn't make linters mad
  2. What shortcomings exist with current approaches? They require me to do a lot of tedious clicking around
  3. What workarounds are you using in the meantime? A lot of tedious clicking around
@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Experience Enhancement Noncontroversial enhancements Help Wanted You can do this labels Mar 28, 2024
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Mar 28, 2024
@abarghoud
Copy link

abarghoud commented Mar 28, 2024

@RyanCavanaugh I'm interested in tackling this. Is this something I can jump in on?

@TulioPintoNeto
Copy link

@abarghoud according to the repository CONTRIBUTING any issue with the label "Help wanted" is allowed to be worked by anyone.

Also, is important to avoid Issue Claiming.

Please feel free to start working in this, you don't have to ask permission as it's labelled with "Help wanted"

@abarghoud
Copy link

@TulioPintoNeto Thanks for pointing it out, I did not know about the issue claiming thing. I forgot to read the contribution guide πŸ€¦β€β™‚οΈ

@TulioPintoNeto
Copy link

TulioPintoNeto commented Mar 29, 2024

Context

@abarghoud I was checking about the way we sort when adding an import (the suggestion on this issue)

Seems like we are sorting by the folder structure, not by the variable name, while Eslint rule is sorting by variable name.

TypeScript import (please note that we have an error in line 2 due to sort-import rule from eslint):

Screen.Recording.2024-03-29.at.21.17.08.mov

Next steps?

With that in mind:

  • Should we use the same order the import command does?;
    OR
  • Should we use the same order as the Eslint rule?

I think it's pertinent, so I'm also bringing this:

Eslint team chose not to auto-fix the order of imports in --fix because this change the way the code could behaviour:
eslint/eslint#11542

I mean, by implementing this behaviour (of reording lines) we can even increase the amount of conflicts with Eslint, as the order of lines could be defined differently for each project on the eslint config. So I'm advocating against it based on the use cases given

@abarghoud
Copy link

@TulioPintoNeto I support the notion of retaining TypeScript's team's design for auto-import ordering. This is especially pertinent considering the availability of multiple ESLint rules that can be configured to alter the sorting method.

I believe the bug that needs fixing is the failure to reorder file names when renaming them. However, I'm concerned that it may not be possible because I think IDEs handle it rather than TypeScript itself.

Wdyt?

@TulioPintoNeto
Copy link

TulioPintoNeto commented Mar 29, 2024

Sorry, I was supposed to mention @andrewbranch.

But to answer you, @abarghoud :

  • This is a experience enhancement, not a bug (not that the nomenclature for this case matters too much hehe);
  • Probably it is possible to achieve this using the TypeScript compiler. The responsibility for intellisense refactorings/formatting/codefixes is within the TypeScript compiler itself. You may find most of the intellisense things in the src/services/ folder;

But the main thing I'm calling out here is that reordering lines when renaming will not always give a consistent output with Eslint (which is the desired according to the use cases provided in the description of this issue). This happens due to the fact that each codebase can set up its own rules for ordering imports.

@andrewbranch
Copy link
Member Author

There are TS Server preferences that control the sort order, and a lot of utilities under ts.OrganizeImports to detect and apply those preferences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experience Enhancement Noncontroversial enhancements Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants