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 Typecast Comments #719

Closed
jacobp100 opened this issue Feb 15, 2017 · 30 comments
Closed

Flow Typecast Comments #719

jacobp100 opened this issue Feb 15, 2017 · 30 comments
Labels
area:comments Issues with how Prettier prints comments lang:flow Issues affecting Flow-specific constructs (not general JS issues) locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! type:bug Issues identifying ugly output, or a defect in the program
Milestone

Comments

@jacobp100
Copy link

jacobp100 commented Feb 15, 2017

In flow, you can typecast with the following,

(obj: Class).property

You can also do this with comments,

(obj /*: Class */).property

However, this gets transformed into,

obj /*: Class */.property

Both are syntactically valid, but only one is recognised by flow. Maybe we'd just want to check comments that start with :?

Happy to help if you decide what direction we should go!

Edit

Also,

const x = (input /*: string */) /*: string */ => {};
// to
const x = (input /*: string */ /*: string */) => {};
@vjeux vjeux added the type:bug Issues identifying ugly output, or a defect in the program label Feb 16, 2017
@vjeux
Copy link
Contributor

vjeux commented Feb 16, 2017

Thanks for the report, we need to fix it!

vjeux added a commit to vjeux/prettier that referenced this issue Feb 16, 2017
The implementation is not super elegant but it seems to work.

Fixes prettier#719
@vjeux
Copy link
Contributor

vjeux commented Feb 16, 2017

@jacobp100 #722

Do you mind running this pull request and try to find crazy annotations that wouldn't work with it? Thanks!

vjeux added a commit to vjeux/prettier that referenced this issue Feb 16, 2017
The implementation is not super elegant but it seems to work.

Fixes prettier#719
@jacobp100
Copy link
Author

Seemed to work!

@vjeux
Copy link
Contributor

vjeux commented Feb 20, 2017

Another idea: use the flow parser to parse comments and have a way to output flow types as comments instead of the ast version.

@vjeux vjeux added the area:comments Issues with how Prettier prints comments label Mar 9, 2017
@jokeyrhyme
Copy link

Related to #204 ?

@jokeyrhyme
Copy link

I've just encountered the Flow comment syntax issues with functions

Here are a few more test cases, I hope these are helpful

https://prettier.github.io v0.22.0

// input
function fnNoArgs () /* : Promise<void> */ {
  return Promise.resolve()
}

// output
function fnNoArgs(/* : Promise<void> */) {
  return Promise.resolve();
}
// input
function fnOneArg (a /* : string */) /* : Promise<string> */ {
  return Promise.resolve(a)
}

// output
function fnOneArg(a /* : string */) /* : Promise<string> */ {
  return Promise.resolve(a);
}
// input
const arrowNoArgs = () /* : Promise<void> */ => {
  return Promise.resolve()
}

// output
const arrowNoArgs = (/* : Promise<void> */) => {
  return Promise.resolve();
};
// input
const arrowOneArge = (a /* : string */) /* : Promise<string> */ => {
  return Promise.resolve(a)
}

// output
const arrowOneArge = (a /* : string */ /* : Promise<string> */) => {
  return Promise.resolve(a);
};

@jokeyrhyme
Copy link

jokeyrhyme commented Apr 23, 2017

Awesome work! It looks like most of the above has been resolved.

As of 1.2.2, the first two examples above are now working perfectly. That is, these are the cases with traditional functions with Flow comments.

Arrow functions (=>) still seem to be problematic, in that the return type Flow comment is being positioned in the wrong place:

// input
const arrowNoArgs = () /* : Promise<void> */ => {
  return Promise.resolve()
}

// output
const arrowNoArgs = () => /* : Promise<void> */ {
  return Promise.resolve();
};
// input
const arrowOneArge = (a /* : string */) /* : Promise<string> */ => {
  return Promise.resolve(a)
}

// output
const arrowOneArge = (a /* : string */ /* : Promise<string> */) => {
  return Promise.resolve(a);
};

@vjeux
Copy link
Contributor

vjeux commented Apr 23, 2017

We now have a much better infrastructure for dealing with comments so we should be able to fix all the issues reported here :)

@brownieboy
Copy link

brownieboy commented Sep 16, 2017

Any update on this one?

Flow comment-style annotations are still being moved from outside to inside the parens on arrow functions for me. I'm using Prettier 1.7.0.

@azz azz added the lang:flow Issues affecting Flow-specific constructs (not general JS issues) label Oct 1, 2017
@duailibe
Copy link
Member

duailibe commented Dec 12, 2017

All the examples off arrow and regular functions are working correctly in master now. You can check out this preview (I copied all the samples in this issue): link

I'll work on the OP example now

@duailibe duailibe added this to the 1.10 milestone Dec 15, 2017
@duailibe duailibe removed this from the 1.10 milestone Jan 9, 2018
@akalin
Copy link

akalin commented Jan 10, 2018

Trying 1.10, this fixes a lot of cases, thanks! One remaining case I ran into is:

    this.state = ({ a, b } /* : AddDemoState */);

link

@suchipi
Copy link
Member

suchipi commented Jan 11, 2018

As a workaround for that particular case, I think you could do:

this.state /* : AddDemoState */ = { a, b };

@akalin
Copy link

akalin commented Jan 15, 2018

Ah, just tried that. flow doesn't seem to like that, I'm guessing because state is a field and not a variable. (I think this is why I wrote it like I did originally.)

@suchipi
Copy link
Member

suchipi commented Jan 15, 2018

I guess you could do this as a workaround then:

const addDemoState /* : AddDemoState */ = { a, b };
this.state = addDemoState;

@suchipi
Copy link
Member

suchipi commented Jan 15, 2018

(We still want to fix this long-term though)

olivoil added a commit to olivoil/react-template that referenced this issue Apr 2, 2018
tribou pushed a commit to tribou/react-template that referenced this issue Apr 3, 2018
* install and run prettier

* ignore style errors from eslint, rely on prettier for that, fixes #68

* fix commitlint task on linux

* add precommit hook

* add .prettierignore file

* add prettier task to package.json

* run prettier task on project

* fix issues related to prettier/prettier#719
@k7n4n5t3w4rt
Copy link

k7n4n5t3w4rt commented May 18, 2018

I'm not sure I'm understanding this correctly but I'm having problems with flow types using the comment syntax.

This:

/*::
type Props = {
  prop1: string,
  prop2: number
}
*/
class App extends Component/*:: <Props> */ {

...

}

is becoming this:

type Props = {
  prop1: string,
  prop2: number
}
class App extends Component<Props> {

...

}

which is valid flow syntax of course. However, I was expecting the comment syntax to be preserved. Am I missing something?

@hawkrives
Copy link
Contributor

@k7n4n5t3w4rt Can you reproduce that bug on the playground (prettier.io)? I ask because I've never personally experienced Prettier changing my type comments to code.

@k7n4n5t3w4rt
Copy link

@hawkrives It seems to be doing the same thing in the playground when the parser option is set to "flow":

https://goo.gl/pL1Nwu

@duailibe
Copy link
Member

@k7n4n5t3w4rt It's a known bug, you can follow it on #3493

@hawkrives
Copy link
Contributor

@k7n4n5t3w4rt Ah, you're using the Flow parser.

If you don't need to use it, you can use the (default) Babylon parser, which also supports Flow types, and doesn't have this bug: playground link

@k7n4n5t3w4rt
Copy link

@hawkrives Aha! I didn't realise I could use the babylon parser, thanks.

@duailibe Thanks for that, I'll keep an eye on it.

@pannous
Copy link

pannous commented Jun 4, 2018

can prettier be used to convert flow annotations into flow comments?
like thus:

prettier --emit-flow-comments   "var test:int=8"
var test/*:int*/=8

@duailibe
Copy link
Member

duailibe commented Jun 4, 2018

@pannous I don't think so, it's out of Prettier's scope, but it's really interesting and someone should totally build a tool that does that.

@suchipi
Copy link
Member

suchipi commented Jun 4, 2018

@pannous you can use https://www.npmjs.com/package/@babel/plugin-transform-flow-comments to do that, though the formatting isn't great:

const { code } = require("@babel/core").transform("var test:int=8", {
  plugins: [require("@babel/plugin-transform-flow-comments")]
});
console.log(code); // "var test\n/*:int*/\n= 8;"

@swac
Copy link
Contributor

swac commented Oct 12, 2018

What's the overall status of this? It seems there are two options, but neither is actually compatible with using Flow's comment syntax.

The first option is to specify "flow" as the parser in Prettier's config file. This preserves Flow compatibility, but it replaces Flow comments with Flow's standard syntax, as shown in @k7n4n5t3w4rt's comment. This effectively defeats the purpose of Flow's comment syntax.

The second option is to stick with the default parser, which retains Flow's comments, but removes the necessary parentheses to satisfy Flow. So something like this:

new (window.Request/*: Class<Request> */)('');

gets turned into this:

new window.Request /*: Class<Request> */("");

This is no longer valid Flow syntax, so Flow throws an error.

Am I correct that because of these two issues together, there's effectively no way to use Flow's comment syntax with Prettier (barring heavy use of prettier-ignore comments)? Although it's possible to rewrite the code like:

const Request/*: Class<Request> */ = window.Request;
new Request('');

this slightly defeats the purpose of Prettier, since it no longer serves as an autoformatter that you can just forget about.

@alexander-akait
Copy link
Member

@swac PR welcome

@swac
Copy link
Contributor

swac commented Oct 12, 2018

@evilebottnawi Apologies if my comment sounded overly critical. I was just trying to sum up the current state of affairs, since it took me a while to piece together everything from this comment thread 😅

@alexander-akait
Copy link
Member

alexander-akait commented Oct 12, 2018

@swac don't worry, we have limited time and contributors to fix all problems, do it yourself it's true the quickest solution 😄

@j-f1 j-f1 added the help wanted We're a small group who can't get to every issue promptly. We’d appreciate help fixing this issue! label Oct 12, 2018
@swac
Copy link
Contributor

swac commented Oct 19, 2018

@evilebottnawi I took a stab at fixing this in #5280 😄

@j-f1 j-f1 added status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! and removed help wanted We're a small group who can't get to every issue promptly. We’d appreciate help fixing this issue! labels Oct 19, 2018
@ikatyang
Copy link
Member

Fixed by #5280.

@ikatyang ikatyang added this to the 1.15 milestone Oct 31, 2018
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area:comments Issues with how Prettier prints comments lang:flow Issues affecting Flow-specific constructs (not general JS issues) locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! type:bug Issues identifying ugly output, or a defect in the program
Projects
None yet
Development

No branches or pull requests