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

isolatedDeclarations fixer inserts unnecessary inline imports #60266

Open
benjaminjkraft opened this issue Oct 18, 2024 · 0 comments · May be fixed by #60267
Open

isolatedDeclarations fixer inserts unnecessary inline imports #60266

benjaminjkraft opened this issue Oct 18, 2024 · 0 comments · May be fixed by #60267

Comments

@benjaminjkraft
Copy link

benjaminjkraft commented Oct 18, 2024

🔎 Search Terms

isolatedDeclarations autofix fix imports

🕗 Version & Regression Information

This is the behavior in every version I tried, and I reviewed the FAQ for entries about isolated declarations. (It seems to go back to the original commits that created the infer-return-type fixers.)

⏯ Playground Link

https://github.com/benjaminjkraft/typescript-autofix-import-bug

💻 Code

// callee.ts
export interface T { myField: string }
export function f(): T {
	return { myField: "hello" }
}

// index.ts
import { f } from "./callee"
export function g() {
	return { wrapper: f() }
}

🙁 Actual behavior

Isolated declarations reports an error for g() (correct). But the fix is to add : { wrapper: import("./callee").T }, which is correct but needlessly unreadable.

🙂 Expected behavior

Instead, the fix should add : { wrapper: T }, and then an appropriate import.

Additional information about the issue

I think the fix may just be to remove the if here? That seems to work in my local testing, although I haven't tested very exhaustively.

I suspect this applies in a few other places (e.g. other calls into the same code with the same pattern), but I didn't look very closely. It also applies to a number of other patterns where you return something exported from elsewhere; this was just the most convenient example to explain.

I may be able to contribute a fix for this (at least once my company reviews the CLA).

benjaminjkraft added a commit to benjaminjkraft/TypeScript that referenced this issue Oct 18, 2024
The fixer has a tendency to create inline imports, whereas toplevel
imports are almost always more readable. And the code already exists,
it's just a matter of using it.

I assume there may be value in applying this in some other places, but
this is the one that's bugging me!

Fixes microsoft#60266.
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.

1 participant