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

Flow: removed support for Flow comments breaks codebases with mixed styles #15024

Closed
mrtnzlml opened this issue Jul 6, 2023 · 13 comments
Closed
Labels
lang:flow Issues affecting Flow-specific constructs (not general JS issues) status:awaiting response Issues that require answers to questions from maintainers before action can be taken

Comments

@mrtnzlml
Copy link

mrtnzlml commented Jul 6, 2023

Prettier 3.0.0
Playground link

--parser=flow | babel-flow | babel

I have a codebase that mixes Flow comments with Flow types (some files can have Flow types, but some files are not transpiled and therefore are using Flow comments). The latest change introduces a breaking change without a clear migration strategy:

Input:

This is just to demonstrate the issue; please read "Expected behavior" point 2 for the main issue. Point 1 is valid as well though.

let a /*: foo */ = x;
let b: foo = x;

Output with parser:flow:

let a: foo = x;
let b: foo = x;

This is incorrect because I wanted to preserve the Flow comments (since I wrote them there for some reason). The same behavior is for parser:babel-flow. Using parser:babel is also not a valid option because it doesn't understand Flow (this is expected):

SyntaxError: Unexpected token (2:6)
  1 | let a /*: foo */ = x;
> 2 | let b: foo = x;
    |      ^
  3 |

Expected behavior:

There are basically 2 problems:

  1. It is not possible to combine Flow comments and Flow types in one file because Prettier effectively destroys the comments. In this case, I would expect Prettier would ignore these comments (the way it was before or the way typescript parser does it). I am OK not to mix the styles in one file, though. More importantly though:
  2. It is not possible to combine Flow comments and Flow types across files:
    • Files that have only Flow comments could be parsed using parser:babel, but that would break on other JS files that contain Flow comments. Please note that both files have extension *.js, so there is no easy way to distinguish these two cases.
    • I could use parser:babel-flow or parser:flow, but that would nuke the Flow comments in the other files where it's not desirable to remove them.

How to deal with files containing multiple Flow styles after this change? 🤔


I am also trying to use prettier-plugin-hermes-parser which seem to work well even though it has its own issues: facebook/hermes#1047

@sosukesuzuki
Copy link
Member

@mrtnzlml Can't you detect by file and directory name?
For example, if files using Flow types is in . /flow-types/*.js and files using Flow comments is in . /flow-comments/*.js,

prettier ./flow-types --parser=flow && prettier ./flow-comments --parser=babel

works fine for you?

@sosukesuzuki sosukesuzuki added status:awaiting response Issues that require answers to questions from maintainers before action can be taken lang:flow Issues affecting Flow-specific constructs (not general JS issues) labels Jul 7, 2023
@mrtnzlml
Copy link
Author

mrtnzlml commented Jul 7, 2023

Hi @sosukesuzuki, this is unfortunately not possible in my case. It's a large monorepo, and there really is a mix of styles in *.js files without any clear line I could draw. A typical example is config files that are not being transpiled (therefore, they are using Flow comments), but in the same project, the rest of the files are transpiled (so normal Flow types).

@github-actions github-actions bot removed the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Jul 7, 2023
@sosukesuzuki
Copy link
Member

@mrtnzlml Honestly, we didn't know of such a use case. And I don't think it is a commonly used way either. So we may never support it. However, I understand that issue. Although it is an ad hoc approach, how about writing a script like the following?

import fs from "node:fs/promises";
import * as prettier from "prettier";
import glob from "fast-glob";

const files = glob.sync("src/**/*.js", { absolute: true });

async function* format() {
  for (const file of files) {
    const data = await fs.readFile(file, "utf-8");
    let result;
    try {
      result = await prettier.format(data, { parser: "babel" });
    } catch {
      try {
        result = await prettier.format(data, { parser: "babel-flow" });
      } catch {
        yield { file, result: null };
      }
    }
    if (data === result) {
      yield { file, result: null };
    } else {
      yield { file, result };
    }
  }
}

for await (const { file, result } of format()) {
  if (result === null) {
    continue;
  }
  fs.writeFile(file, result);
}

The script first formats using the babel parser, and if that fails, formats using the babel-flow parser. I know the execution time will be longer, but does this solve your problem?

@sosukesuzuki sosukesuzuki added the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Jul 8, 2023
@fisker
Copy link
Member

fisker commented Jul 8, 2023

Maybe pretter . --parser babel --ignore-unknown || pretter . --parser babel-flow can so the same thing as @sosukesuzuki suggestted ?

@julienw
Copy link

julienw commented Jul 13, 2023

I do have a similar issue and it's not clear to me how to fix it.

  • I use flow comments in nodejs scripts because I want them to be able to be run directly.
  • I use the @flow pragma so that they're checked by flow
  • Because of this the babel parser switches to "flow mode" and rewrites all comments to types, and then the file isn't runnable by node directly anymore.

By chance, is there a way to tell prettier to not rewrite the comments even in such files?

update : we were using the parser babel-flow explicitely but this doesn't change the issue.

@julienw
Copy link

julienw commented Jul 13, 2023

We were also using some special comment to support iterators properly with flow:

  /*::
  @@iterator(): * {
    // $FlowFixMe ignore Flow error about Symbol support
    return this[Symbol.iterator]()
  }
  */

This tells flow about the existence of an iterator. But this isn't proper javascript code, that's why it's inside a flow comment. But now prettier tries to parse it and fails. I tried all parsers (babel, babel-flow and flow). It would be good to ignore the comments as before, if that's possible.

@thorn0
Copy link
Member

thorn0 commented Jul 21, 2023

@mrtnzlml

It is not possible to combine Flow comments and Flow types across files

Use config overrides to specify which files should be processed with which parser.

@julienw Use any non-Flow parser for your files. E.g. meriyah.

@github-actions
Copy link

This issue has been automatically closed because there has been no response to our request for more information from the original author. With only the information that is currently in the issue, we don't have enough information to take action. Please reach out if you have or find the answers we need so that we can investigate further.

@julienw
Copy link

julienw commented Aug 24, 2023

@julienw Use any non-Flow parser for your files. E.g. meriyah.

The file that uses @@iterator in a comment otherwise uses "usual" flow elsewhere. The change is really painful to us and I can't see a way of upgrading prettier in this current form, except ignoring this file completely so that it's not parsed.

@julienw
Copy link

julienw commented Aug 24, 2023

@julienw Use any non-Flow parser for your files. E.g. meriyah.

The file that uses @@iterator in a comment otherwise uses "usual" flow elsewhere. The change is really painful to us and I can't see a way of upgrading prettier in this current form, except ignoring this file completely so that it's not parsed.

I managed to make it work finally by switching to the flow parser for this file instead of babel-flow. update: oh no this doesn't work because this still rewrites the comments into invalid code... update 2: using babel-ts makes me get away with this, because Flow syntax in this file is typescript compatible, and babel-ts doesn't rewrite comments. Still it's a bit hacky...

For my other files with only comments, I used espree (too bad we can't use babel because that one switches to babel-flow automatically with the @flow pragma).

@fisker
Copy link
Member

fisker commented Aug 25, 2023

I forgot why we don't just treat flow comments as normal comments. It doesn't make sense to me now.

@fisker
Copy link
Member

fisker commented Aug 25, 2023

Found #13687 (comment)

@julienw
Copy link

julienw commented Aug 25, 2023

There's also some context in the blog post: https://prettier.io/blog/2023/07/05/3.0.0.html#flow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang:flow Issues affecting Flow-specific constructs (not general JS issues) status:awaiting response Issues that require answers to questions from maintainers before action can be taken
Projects
None yet
Development

No branches or pull requests

5 participants