-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add heuristics to detect type aliases that has complex typeparameters #10901
Conversation
typeParams.length > 1 && | ||
!!getLast(typeParams).default && | ||
typeParams.every((param) => !!param.constraint) | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we only have the condition to fix just #10857, but I think we can improve here based on feedback in the future.
src/language-js/print/assignment.js
Outdated
Boolean(getLast(typeParams).default) && | ||
typeParams.every((param) => Boolean(param.constraint)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why check .default
only for the last type parameter?
Boolean(getLast(typeParams).default) && | |
typeParams.every((param) => Boolean(param.constraint)) | |
typeParams.some((param) => param.constraint || param.bound || param.default) |
param.bound
is for Flow (playground).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot about param.bound
, thank you.
Why check .default only for the last type parameter?
Because the default value of type parameters can only appear on last param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the default value of type parameters can only appear on last param
It's not the case.
update: oh, sorry, you were right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this?
type Bar<T = string, S> = {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, TS doesn't accept that. I thought S
in that situation would default to any
, but it doesn't work that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, I think it's better to break the LHS even if at least one type parameter has a constraint or a default (some
, not every
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I have no strong opinion
Please add a changelog entry |
@@ -0,0 +1 @@ | |||
run_spec(__dirname, ["flow", "babel"]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's run flow test with babel-flow
instead of babel
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
Fixes #10857
Checklist
changelog_unreleased/*/XXXX.md
file followingchangelog_unreleased/TEMPLATE.md
.✨Try the playground for this PR✨