Skip to content

Isolated declarations quick fix results in redundant type arguments when the RHS is a new expression with explicit type arguments #59768

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

Open
1 task done
blickly opened this issue Aug 26, 2024 · 4 comments
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Milestone

Comments

@blickly
Copy link
Contributor

blickly commented Aug 26, 2024

Acknowledgement

  • I acknowledge that issues using this template may be closed without further explanation at the maintainer's discretion.

Comment

// @declaration: true
// @isolatedDeclarations: true
export const s = new Set<string>();

After generating and applying a quick-fix, the current resulting code is:

export const s: Set<string> = new Set<string>();

An ideal fix would remove the redundant type arguments in the RHS expression when adding the same type arguments to the suggested type annotation, resulting in something like:

export const s: Set<string> = new Set();
@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases labels Aug 30, 2024
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Aug 30, 2024
@iisaduan
Copy link
Member

The fix should consider adding another ID fix option that will both add the annotation and do the RHS removal, instead of changing the current quickfixes to always removing the type parameters. The default behavior of lint rules such as https://typescript-eslint.io/rules/consistent-generic-constructors/ give codefixes that undo the ID annotation, so it would be nice to leave a codefix that could leave ID compatible with that rule

@blickly
Copy link
Contributor Author

blickly commented Sep 14, 2024

Thanks for pointing that out!

It also seems like if you prefer the non-redundant style, you could set your project to use the type-annotation setting and potentially run eslint --fix before running the isolated declarations quick-fixes to get a less redundant result without needing to change how the isolated declarations quick-fixer works. To me, that indicates that this issue is lower priority than it would have been otherwise.

(I also wonder if the default for typescript-eslint.io/rules/consistent-generic-constructors/ would make more sense to be type-annotation now that isolatedDeclarations exists, but that's probably a separate discussion)

@iisaduan
Copy link
Member

(I also wonder if the default for typescript-eslint.io/rules/consistent-generic-constructors/ would make more sense to be type-annotation now that isolatedDeclarations exists, but that's probably a separate discussion)

If the particular declaration doesn't require the type annotation for ID (such as in a codebase without ID on), it seems to make a lot of sense to leave the default on the RHS, as with inference, you often don't need the type annotation at all. But like you said, this is probably a discussion that should/could be had over at their repo

It also seems like if you prefer the non-redundant style,..

😄 in the PR to adopt ID, I removed that rule out of the TS codebase today in 6296b14, and while filing issues for possible ID improvements that I noticed while making that PR, I found this issue. It would have been nice to have a one stroke codefix to update just the ID related annotations in this way.

@blickly
Copy link
Contributor Author

blickly commented Sep 16, 2024

Great point. In some cases the RHS style is clearly cleaner, even though there are other cases where it clashes with ID.

I'm still interested in the idea of having the ID quickfix move the type arguments from RHS->LHS rather than duplicate it. I'm a little hesitant to include an additional mode to generate the duplicate annotations, since it's not clear to me that the "duplicated type arguments" style is really something that folks actively support vs. just being an accidental quirk of how the current implementation of the https://typescript-eslint.io/rules/consistent-generic-constructors/ rule interacts with ID. I've filed typescript-eslint/typescript-eslint#10001 over there to try to get more clarity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Projects
None yet
Development

No branches or pull requests

3 participants