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

Support conditional type and infer type in Flow #14573

Merged
merged 8 commits into from Mar 28, 2023
Merged

Support conditional type and infer type in Flow #14573

merged 8 commits into from Mar 28, 2023

Conversation

SamChou19815
Copy link
Contributor

@SamChou19815 SamChou19815 commented Mar 23, 2023

Description

This PR adds support for printing conditional types and the companion infer type using flow-parser. It uses the existing printTernary helper function that's already implemented for TS. Since Flow's parser produces AST with the same name, it automatically works. For infer type, we also reuse the existing printTypeParameter function.

For testing, I copy the conditional type test suite from TS, and deleted some syntax Flow doesn't support yet (type assertion), and run both Flow and TS prettier on it and ensure they are consistent. I moved the comment one into another suite, because the results are different.

Checklist

  • I’ve added tests to confirm my change works.
  • (N/A) (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@SamChou19815 SamChou19815 marked this pull request as ready for review March 24, 2023 19:42
@@ -152,7 +152,11 @@ function printTypeParameter(path, options, print) {
parts.push(name);

if (node.bound) {
parts.push(printTypeAnnotationProperty(path, print, "bound"));
if (node.usesExtendsBound) {
parts.push(" extends ", print(["bound", "typeAnnotation"]));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node.bound is skipped, better not to do that, there could be comments, cursors.

This won't work?

parts.push(" extends ", print("bound"));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I understand the problem, we should still use printTypeAnnotationProperty. I'll fix it and add comments to explain why it's needed and how it works on monday.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a case

Prettier pr-14573
Playground link

--parser flow

Input:

type A = () => infer R extends/* comment */ string

Output:

Error: Comment "comment" was not printed. Please report this error!
    at dn (https://deploy-preview-14573--prettier.netlify.app/lib/standalone.js:29:5817)
    at Ze (https://deploy-preview-14573--prettier.netlify.app/lib/standalone.js:29:7308)
    at async kn (https://deploy-preview-14573--prettier.netlify.app/lib/standalone.js:29:11045)
    at async ju (https://deploy-preview-14573--prettier.netlify.app/lib/standalone.js:34:581)
    at async formatCode (https://deploy-preview-14573--prettier.netlify.app/worker.js:159:12)
    at async handleFormatMessage (https://deploy-preview-14573--prettier.netlify.app/worker.js:83:24)
    at async self.onmessage (https://deploy-preview-14573--prettier.netlify.app/worker.js:38:14)

Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except where Fisker reviewed


if (key === "checkType" && parent.type === "ConditionalTypeAnnotation") {
return true;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loos like we are missing cases, it's bad we are not sharing logic between TS and Flow.
Maybe better extract some code first to reuse?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the logic and pass the test. I'm not sure how to better extract the code, since there are very little code sharing currently in the file.

My personal opinion is that the existing fallthrough logic for TS nodes already made it harder to understand, and I actually like the more explicit logic among those Flow nodes that don't use fallthrough...

@fisker fisker changed the base branch from next to main March 27, 2023 08:35
@fisker fisker merged commit b7ab340 into prettier:main Mar 28, 2023
30 checks passed
medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Feb 13, 2024
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 this pull request may close these issues.

None yet

3 participants