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

Show flow issues together with eslint issues #324

Closed
kasperpeulen opened this issue Aug 1, 2016 · 20 comments
Closed

Show flow issues together with eslint issues #324

kasperpeulen opened this issue Aug 1, 2016 · 20 comments

Comments

@kasperpeulen
Copy link
Contributor

I was wondering if better flow integration is something you guys consider.

Flow finally has Windows support: 🎉
https://www.flowtype.org/blog/2016/08/01/Windows-Support.html

I often notice flow issues too late, when I work on a create react app. The eslint issues are always in my face at the terminal, at the console, and in my editor (Webstorm). So I never miss out on them.

However, for flow issues, I have to run flow manually. Sadly, Webstorm doesn't show any flow issues. And also this project doesn't show any flow issues to me. Sometimes I find a bug after quite some time, and then I notice that flow would have already seen this for ages, but I just forgot to run flow.

I would love it if the flow issues are also seen in the terminal and console.

@lacker
Copy link
Contributor

lacker commented Aug 1, 2016

I would love this as well, especially if we can make it run in a way that is unobtrusive for people who are not using Flow.

@gaearon
Copy link
Contributor

gaearon commented Aug 1, 2016

I also would love this but I’m not totally sure how to approach this best. Proof of concept is welcome if you'd like to work on one.

@torifat
Copy link
Contributor

torifat commented Aug 2, 2016

@gaearon I would like to work on a POC. One question though. Do we want to include flow-bin or look for the global flow?

@gaearon
Copy link
Contributor

gaearon commented Aug 2, 2016

@torifat I think including flow-bin is the way to go. Note that I have close to zero experience with it.

@torifat
Copy link
Contributor

torifat commented Aug 2, 2016

@gaearon no problem. We can revert it easily if any flow veteran suggests otherwise in the future 😄

@torifat
Copy link
Contributor

torifat commented Aug 2, 2016

@gaearon It seems we need a .flowconfig in the root (template) directory. There is a open related issue.

facebook/flow#389

@gaearon
Copy link
Contributor

gaearon commented Aug 2, 2016

For now this is the only way: https://github.com/facebookincubator/create-react-app/blob/master/template/README.md#add-flow

But let’s skip that and focus on integration. We can figure out how to “provide” the config later.

@torifat
Copy link
Contributor

torifat commented Aug 2, 2016

I was looking at the current lint code and found that we are using eslint-loader. So, it would be great if we can follow the similar approach for flow too. I was looking for a flow-loader and found this. But, this one doesn't pass any error or warning to the emitter and also it's not a loader.

In flow there's an option for getting json output of any command like flow status --json. So, I think I can make a separate flow-loader and integrate it here.

@gaearon what's your thought?

Related Issues:

@gaearon
Copy link
Contributor

gaearon commented Aug 2, 2016

This sounds reasonable.

@gabelevi
Copy link

gabelevi commented Aug 2, 2016

@torifat - The --json output doesn't include the pretty-printed error, but I have written some JS to handle consuming the JSON output and pretty printing it. I really need to stick it into a npm package, but until then it lives here: https://github.com/facebook/flow/blob/master/tsrc/flowResult.js.

@torifat
Copy link
Contributor

torifat commented Aug 2, 2016

UPDATE

I have already started working with the loader: https://github.com/torifat/flowtype-loader

Any kind of idea/suggestions are welcome 😄

Hopefully I will be able to finish it within tomorrow.

@jntn
Copy link

jntn commented Aug 23, 2016

@torifat any update on this? It would be cool to get this working 😄

@gaearon
Copy link
Contributor

gaearon commented Aug 23, 2016

We need somebody from the flow team to look at flowtype-loader. cc @gabelevi

@torifat
Copy link
Contributor

torifat commented Aug 23, 2016

@jntn It's working, you can check it by pulling my branch. I'm also waiting for feedbacks.

@vinpac
Copy link

vinpac commented Sep 2, 2016

Any update on this?

@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2016

@thejameskyle contacted @torifat, waiting for them to come up with a plan. Flow team is definitely interested in this and will pursue making this happen.

@amilajack
Copy link

amilajack commented Sep 24, 2016

I'm working on an eslint plugin that reports flow errors as eslint errors:

ESLint Flow Demo

https://github.com/amilajack/eslint-plugin-flowtype-errors

@kasperpeulen
Copy link
Contributor Author

@amilajack Awesome, I hope something like that will be used in create react app

@juanpaco
Copy link

juanpaco commented Aug 1, 2017

As a stop-gap measure while this is being worked on, I came across this: https://www.npmjs.com/package/flow-watch. It's one more thing to kick off from the command line, but it alerts to errors at save time.

@gaearon
Copy link
Contributor

gaearon commented Jan 8, 2018

We're probably not going to pursue this. It doesn't seem like the Flow team is actively interested in such integrations, and trying to do something like this is like swimming against the current while APIs change.

@gaearon gaearon closed this as completed Jan 8, 2018
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants