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

Make the generated app work if you flowcheck it #72

Closed
lacker opened this issue Jul 22, 2016 · 45 comments
Closed

Make the generated app work if you flowcheck it #72

lacker opened this issue Jul 22, 2016 · 45 comments

Comments

@lacker
Copy link
Contributor

lacker commented Jul 22, 2016

It would be nice if it were possible to add flow to the default project. I tried a minimal flow setup, adding a blank .flowconfig to the root and // @flow to App.css. Four errors:
screen shot 2016-07-21 at 10 06 32 pm

For the first two I might be configuring things wrong? The second two seem like flow doesn't know how to handle the non-js imports that webpack is handling for us. I'm not sure what the right way to resolve this.

I was using flow version 0.29.0

@keyz
Copy link
Contributor

keyz commented Jul 22, 2016

I think the first two are fine if we setup .*/node_modules/.* as an ignored path in .flowconfig. The second two require a name mapper: https://flowtype.org/docs/modules.html#css-modules-with-webpack

@gaearon
Copy link
Contributor

gaearon commented Jul 22, 2016

Indeed our case we’d need to map .css to modules that don’t export anything, and all other files (e.g. images) to modules that export strings. This is similar to what React Native does.

I am, however, not very happy with creating configs on user’s behalf right in their directory even if they don’t use Flow. The point of this project is that there is no user configuration, at least for as long as you can avoid it.

What if flow init could somehow “autodiscover” our initial config, and use it instead of the default one? This way we wouldn’t create any files by default, but when the user runs Flow for the first time, they get it correctly preconfigured. Since this is done on demand, we have a chance to keep improving the initial config, and even people who created apps a long time ago would get an up-to-date initial config when they start using Flow.

gaearon added a commit that referenced this issue Jul 22, 2016
See #72 for details
@gaearon
Copy link
Contributor

gaearon commented Jul 22, 2016

As a temporary workaround, I added these stubs: 6885094.
Now I see no errors with a config like this:

[libs]
./node_modules/fbjs/flow/lib

[options]
esproposal.class_static_fields=enable
esproposal.class_instance_fields=enable

module.name_mapper='^\(.*\)\.css$' -> 'react-scripts/config/flow/css'
module.name_mapper='^\(.*\)\.\(jpg\|png\|gif\|eot\|svg\|ttf\|woff\|woff2\|mp4\|webm\)$' -> 'react-scripts/config/flow/file'

suppress_type=$FlowIssue
suppress_type=$FlowFixMe

And I wish this config was inside react-scripts somehow, at least until flow init.

@mxstbr
Copy link
Contributor

mxstbr commented Jul 22, 2016

What about having a command like npm run flow which adds that config and does flow init? Maybe too much?

@gaearon
Copy link
Contributor

gaearon commented Jul 22, 2016

I’d like something like npm run add flow, npm run add relay, etc, for the future.
For now I’ll document this workaround in How To.

gaearon added a commit that referenced this issue Jul 22, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 22, 2016

I added some instructions to the howto guide for now: https://github.com/facebookincubator/create-react-app/blob/master/template/README.md#adding-flow

Longer term, I think we should include Flow by default and Flow should have a way of extending configs like

import 'react-scripts/configs/flow'

for basic settings.

gaearon added a commit that referenced this issue Jul 22, 2016
@kasperpeulen
Copy link
Contributor

I would love flow being included by default 👍. Does that mean that we would also get flow errors in the chrome console and terminal? Flow helps me a lot with fixing bugs in javascript.

People that don't like flow (for some reason) probably wouldn't even notice it, as they don't include the /* @flow */ comment on top of their file.

@jamiebuilds
Copy link

So there are a number of tasks that we have created in order to simplify the setup of Flow in create-react-app.

Right now the .flowconfig that you need to add to get started is a bit difficult. As @gaearon mentioned before it looks like this:

[libs]
./node_modules/fbjs/flow/lib

[options]
esproposal.class_static_fields=enable
esproposal.class_instance_fields=enable

module.name_mapper='^\(.*\)\.css$' -> 'react-scripts/config/flow/css'
module.name_mapper='^\(.*\)\.\(jpg\|png\|gif\|eot\|svg\|ttf\|woff\|woff2\|mp4\|webm\)$' -> 'react-scripts/config/flow/file'

suppress_type=$FlowIssue
suppress_type=$FlowFixMe

There's a couple things we want to do to get it down to zero config.

  • Ignoring node_modules by default, and have a happy path for installing flow-typed definitions. This eliminates the need to have the path in [libs] and the surpress_type's.
  • Having sensible defaults for different file extensions. We can't support 100% code coverage on something like css-modules but we can support the way all these files are used within create-react-app. This eliminates the module.name_mapper fields.
  • Potentially enable >= stage 2 esproposals by default.

We also want to improve the experience of Flow other than the initial setup.

  • Integrate with npm start for display type warnings like eslint does right now.
  • Hook into npm install for adding flow-typed definitions automagically without pain (or maybe some other solution than npm install, idk yet)

We discussed allowing the repo to have no .flowconfig file, but having this constraint improves the text editor experience so we're punting on that for now.

As for enabling Flow in create-react-app projects themselves, we would be interested in seeing that happen. As for if it's enabled on all files by default is up to the community to decide on what they want most. We don't want to force ourselves on anyone, but adding // @flow on the top of every file is tedious.

If anyone has questions about this let me know, @gabelevi and I will be working through these.

@kasperpeulen
Copy link
Contributor

Integrate with npm start for display type warnings like eslint does right now.

😍😍😍 There is someone already investigating this I believe:
#324

Hook into npm install for adding flow-typed definitions automagically without pain (or maybe some other solution than npm install, idk yet)

😍😍😍

We don't want to force ourselves on anyone, but adding // @flow on the top of every file is tedious.

This is actually what I do in all my flow projects, is there another way?

@jamiebuilds
Copy link

jamiebuilds commented Aug 2, 2016

We don't want to force ourselves on anyone, but adding // @flow on the top of every file is tedious.

This is actually what I do in all my flow projects, is there another way?

Yes, you can add --all to the cli command or all=true to your .flowconfig under [options]

@lacker
Copy link
Contributor Author

lacker commented Aug 2, 2016

I find it tedious to put // @flow at the top of files, but I also find it tedious when I run Flow with --all and it complains about things that aren't really problems. It's especially bothersome if that's someone's first experience with Flow.

What we ended up doing with eslint in create-react-app is configuring things in a very conservative mode, only reporting problems in cases where we can be highly confident it's a real problem. Would something like that be possible with Flow?

Also I would like to volunteer to help out testing any development build with a "flow defaults to on" experience!

@kasperpeulen
Copy link
Contributor

@thejameskyle I could not get the --all options to work. It also doesn't seem to be listed here:
https://flowtype.org/docs/cli.html

The all=true option seem to do something. In the sense that it taking my mac already 4 min to complete hehe, and still not completed, and making all kind of noises that I have never heard lol :P Is it analyzing now my node_modules as well?

@gabelevi
Copy link

gabelevi commented Aug 2, 2016

--all isn't available on all the commands. Like flow status --all doesn't exist, since it's not clear what it should do if there's already a flow server running without the --all command. But flow server --all, flow check --all, and flow start --all should all exist.

And yeah, it will check everything, including node_modules :)

@webdesserts
Copy link

It would be nice if the type checking was weak by default, but you could add // @flow to make the file strongly typed. We use flow throughout our apps and like to trickle // @flow weak on files as we're trying to clean them up and only ever use // @flow when the code is mission critical.

@nmn
Copy link

nmn commented Aug 3, 2016

@jameskyle the ignoring of node_modules by default can be problematic. Usually, we want flow to ignore the errors but still read the type information. I tend to publish my NPM modules with their flow types. This way you don't need to get the flow-typed module separately (see react-time ago)

I hope that use-case doesn't break.

@dakt
Copy link

dakt commented Aug 24, 2016

+1 to include flow by default

@gaearon
Copy link
Contributor

gaearon commented Sep 3, 2016

This is mostly fixed now. I updated instructions in #567.
fbjs is the only remaining issue there and it isn’t strictly related to CRA setup so closing.

@gaearon gaearon closed this as completed Sep 3, 2016
@SpicyPete
Copy link

I just started a new project with create-react app, and for flow to work I had to include the bit in the documentation,

[ignore]
<PROJECT_ROOT>/node_modules/fbjs/.*

As well as

[options]
suppress_type=$FlowIssue

@jayphelps
Copy link
Contributor

jayphelps commented Oct 28, 2016

Integrate with npm start for display type warnings like eslint does right now.

@thejameskyle did you end up not being able to do this? Doesn't appear to happen--I need to run flow manually--I may have missed some sort of configuration though.

@gaearon
Copy link
Contributor

gaearon commented Oct 28, 2016

@jayphelps No, this is not included. There is a proof of concept in #350 and @gabelevi expressed interest in having an official Flow loader for webpack but so far I haven't seen any progress on this.

@rricard
Copy link
Contributor

rricard commented Nov 1, 2016

OBSOLETE You don't need any stub now!

Note: with the latest react-scripts there is no flow stubs for css and files. Now you will need to redirect to the stubs made for jest:

module.name_mapper='^\(.*\)\.css$' -> 'react-scripts/config/jest/CSSStub'
module.name_mapper='^\(.*\)\.\(jpg\|png\|gif\|eot\|svg\|ttf\|woff\|woff2\|mp4\|webm\)$' -> 'react-scripts/config/jest/FileStub'

Here is my currently working, out of the box, .flowconfig:

[ignore]
<PROJECT_ROOT>/node_modules/fbjs/.*

[libs]
./node_modules/fbjs/flow/lib

[options]
esproposal.class_static_fields=enable
esproposal.class_instance_fields=enable

module.name_mapper='^\(.*\)\.css$' -> 'react-scripts/config/jest/CSSStub'
module.name_mapper='^\(.*\)\.\(jpg\|png\|gif\|eot\|svg\|ttf\|woff\|woff2\|mp4\|webm\)$' -> 'react-scripts/config/jest/FileStub'

suppress_type=$FlowIssue
suppress_type=$FlowFixMe

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

Note: with the latest react-scripts there is no flow stubs for css and files. Now you will need to redirect to the stubs made for jest:

This sounds wrong. You shouldn't need to redirect any stubs, it should "just work".
Have you tried removing module name mappers completely?

Reopening because I'm confused and need a confirmation that it works without any mappers in the most recent Flow version.

@gaearon gaearon reopened this Nov 20, 2016
@rricard
Copy link
Contributor

rricard commented Nov 20, 2016

@gaearon Didn't tried without the mappers actually. I just followed the README generated by create-react-app a while ago. This seems fixed now though (the template doesn't ask you to map anymore). I think you can close this safely and dismiss my last comment!

@rricard
Copy link
Contributor

rricard commented Nov 20, 2016

I'll still try with the latest flow version.

@rricard
Copy link
Contributor

rricard commented Nov 20, 2016

Ok, nice, it works with this minimal .flowconfig:

[ignore]
<PROJECT_ROOT>/node_modules/fbjs/.*

[libs]
./flow-typed

I also ran this to get type annotations for jest (explaining the [libs] section):

npm i -g flow-typed
flow-typed install jest@17.0.0 --flowVersion 0.35

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

@rricard Could you update our Flow instructions to match your experience (e.g. Jest thing).

@gaearon gaearon added docs and removed issue: bug labels Nov 20, 2016
@rricard
Copy link
Contributor

rricard commented Nov 20, 2016

That bring me to an another point: maybe we should do a flow-typed annotation for react-scripts that aliases to jest. Because, since jest is not explicitly depended on in the package.json, you will not be able to just flow-typed install and get the annotations just like this

@rricard
Copy link
Contributor

rricard commented Nov 20, 2016

So, I think documenting the flow-typed procedure could be a good thing, but first we have to figure out the react-scripts flow-typed annotation... Going to do this just now!

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

Because, since jest is not explicitly depended on in the package.json, you will not be able to just flow-typed install and get the annotations just like this

It would be nice if flow-typed knew about react-scripts and looked one package deeper.
cc @thejameskyle

@rricard
Copy link
Contributor

rricard commented Nov 20, 2016

@gaearon @thejameskyle I'm working on a short-term solution PR for that in flow-typed

@rricard
Copy link
Contributor

rricard commented Nov 20, 2016

Here: flow-typed/flow-typed#467

@rricard
Copy link
Contributor

rricard commented Nov 20, 2016

@gaearon Otherwise, I was wondering if you would be interested in adding flow out of the box to create-react-app? That would mean having flow type errors showing in the start/compile output like eslint does. This would be completely optional since you would need to add /* @flow */ to the file to see it typechecked. This also means we would provide a default .flowconfig in the template and add a script triggered at postinstall to download flow-typed annotations after adding some packages.

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

I'd definitely be interested. There is a PR with a proof of concept in #350. I reached out to the Flow team several times asking for their feedback on the Webpack loader that this PR used but unfortunately they never got back to me.

If you're interested in taking this proof of concept and making it production-ready (maybe looking at other official Flow integrations and taking lessons from them) this is a good contribution opportunity.

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

This also means we would provide a default .flowconfig

Nah. We'd create it from the loader the first time it sees a flow annotation.

@rricard
Copy link
Contributor

rricard commented Nov 20, 2016

@gaearon Well, I'll take a look at all of this... One last question, so if you don't want the .flowconfig right away, I imagine, we'll only download them from the loader if we see a flow annotation as well, right?

@rricard
Copy link
Contributor

rricard commented Nov 20, 2016

@gaearon I don't know either how the flow team would approach this problem, but I would work around flow-bin and not use a loader. Flow works project-wide and, to me, it doesn't make much sense as a loader. Maybe a webpack plugin would work, but I want to see first where I can go without it...

@rricard
Copy link
Contributor

rricard commented Nov 20, 2016

That way we only depend on flow-bin, a facebook-supported dependency with custom code wrapping its execution in an another facebook-supported project. So you don't end up depending on my weird custom package or whatever...

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

@rricard It being a loader would help integrating into npm start IMO. I don't advocate it actually behaving like a loader, but rather using that as an opportunity to know when to trigger re-checks. You'd still debounce its calls and wouldn't really use the sources.

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

But yea, WP plugin could work too. As long as you have a way to know when to trigger re-checks (or does Flow do this automatically anyway? I have no idea.)

@rricard
Copy link
Contributor

rricard commented Nov 20, 2016

@gaearon I absolutely see why a loader has its advantages. Plugins in my opinion would work too.

@rricard
Copy link
Contributor

rricard commented Nov 20, 2016

I don't know when flow updates exactly but once it's started, you just ask for the typechecking results and it's right there with no delay. So the question is when to add the flow errors to the output.

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

@rricard And when to "re-trigger" the output due to Flow even if webpack didn't re-trigger it by itself.

@rricard
Copy link
Contributor

rricard commented Nov 20, 2016

@gaearon Starting to see a bit more precisely what is going on here. I'll try something using flow-bin and a plugin that I'll place in react-dev-utils. From there, I think I'll be able to get you a POC and probably something publishable soon after. If the plugin approach doesn't work, i think that creating a loader from my plugin attempt will not be hard.

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

Cool, thanks for looking at it!

@gaearon
Copy link
Contributor

gaearon commented Feb 11, 2017

Going to close as it should be fine now.

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

No branches or pull requests