-
Notifications
You must be signed in to change notification settings - Fork 12.9k
fix(40609): Crash on extract to type alias #40820
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
Conversation
Why were duplicate type parameters causing the crash? |
415592f
to
adafbbc
Compare
adafbbc
to
30e678c
Compare
The type Foo<T> = {
fn: (a: T, b: T, c: T) => void;
}
|
Yes, that makes sense. Thanks for the explanation and the PR! |
I'm still not clear on why it crashes but this seems to fix another future bug. |
At some point it would be good to add a variety of tests that pump template and pattern literal types through codefixes/refactors. When @jessetrinity and I were debugging this, we definitely saw the formatting scanner get tripped up on that node. I still think there might be a missing condition to |
@andrewbranch I'm ok to revert these changes, in favor of adding changes to formatter. |
Strictly speaking it doesn't fix the template type bug, #40738 did. Previous to that, extract type on type a = `${a}` would cause a crash. You should actually be able to remove the string templates from the tests in this PR and still get the crash previous to this change. |
Oh no, these changes are clearly correct, I was just surprised they fixed the bug.
Ah, that makes more sense 👍 |
@jessetrinity Yep, I was waiting for the merging of this PR., because my first PR contained changes related to missing |
@andrewbranch @jessetrinity Does that make sense to drop the test |
I thought it was good to have a test on the code that surfaced the bug, but maybe it's a little confusing since there were two bugs..? If anything, we might want a more minimal test for the template string issue covering with span: type a = \*a*\`${extractMe}`\*b*\ |
Fixes #40609