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

Add flow typechecking as a webpack plugin #1152

Closed
wants to merge 75 commits into from

Conversation

rricard
Copy link
Contributor

@rricard rricard commented Dec 4, 2016

cra-flow-opt

This will only run if a @flow annotation is seen in an user's file:

  • At the first @flow file found:
    • Write a .flowconfig if none exists
    • Run flow-typed install
    • Add some custom flow-typed installs (implicit jest behind react-scripts for instance)
  • When a file with an @flow comment changes during a compilation:
    • Run a flow check
    • If there are some type errors in flow: show warnings
    • If flow itself crashes: show error
  • Refactor

Followup goals:

  • Return-code-based success of the command (instead of looking for "No Errors!"
  • Detect if a global flow installation exists, if yes, check its version, if not equal to CRA's version, show a warning that compilation may be slowed down

1__yarn_start__node_

  • Ensure the first flow status run was done after complete init (especially flow-typed)
  • Add flow-typed dir to .gitignore during the first run
  • Add to end-to-end testing

Followup of a discussion in #72

@gaearon
Copy link
Contributor

gaearon commented Dec 4, 2016

Neat! Going to look later this week. Want to make a GIF 😄 ?

@rricard
Copy link
Contributor Author

rricard commented Dec 4, 2016

@gaearon Not done yet, will do a gif when I'm completely happy about it!

@rricard
Copy link
Contributor Author

rricard commented Dec 4, 2016

Btw, for now, the code is pretty awful callback hell-spaghetti, I have to admit I got used to promises, almost forgot about working like this!

@gaearon
Copy link
Contributor

gaearon commented Dec 4, 2016

We depend on Node >= 4 so you should be able to use Promises if you want to.

@rricard
Copy link
Contributor Author

rricard commented Dec 4, 2016

@gaearon Issue is, webpack and vanilla Node API doesn't really expose Promises. I usually use some form of promisifier to handle Node API... I don't want to add more deps than useful.

@rricard
Copy link
Contributor Author

rricard commented Dec 4, 2016

Okay, I checked, there are arrow functions elsewhere in this codebase, I'm going to need that as well!

@rricard rricard changed the title [In progress] Add flow typechecking as a webpack plugin Add flow typechecking as a webpack plugin Dec 5, 2016
@rricard
Copy link
Contributor Author

rricard commented Dec 5, 2016

@gaearon It's way better now! I'm more satisfied with it... But I want to see what your review will show... Maybe one problem we may have here is that the flow check is global and not per-file. I can work on that maybe a bit later but I see one issue in not having the global output, we'd lose warnings in test files so I have to give it a few more thoughts before doing that.

Thanks and good luck for the review!

@gaearon
Copy link
Contributor

gaearon commented Dec 5, 2016

One thing to keep in mind is the scenario where changing file A adds or removes an error from file B. Does this work now?

@rricard
Copy link
Contributor Author

rricard commented Dec 5, 2016

@gaearon As long as A is covered by flow, typechecking project-wide will be recomputed, so we'll see issues being propagated to B if this is the case.

.then(newOutput => {
flowOutput = newOutput;
// Output a warning if flow failed
if (flowOutput.indexOf('No errors!') < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does Flow have IDE-friendly mode outputting JSON or something else parseable so that you don't need to search strings? How does Nuclide Flow integration work?

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 can output JSON with flow status --json. Drawback is, I have to redo the output that is already nice and colored. However, I agree comparing with "No errors!" is bad. Maybe I should use the standard error code, which represents a better feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, makes sense!

Copy link
Contributor Author

@rricard rricard Dec 5, 2016

Choose a reason for hiding this comment

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

Concerning Nuclide, they use a non documented package that handles the process lifecycle that is, by the way, pretty cool. However, as said before, it is not documented and may be pretty unstable, the advantage here is that we only depend on flow-bin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, definitely want to go with a lighter way to do it for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Return code-based solution works super well!

@gaearon gaearon added this to the 0.9.0 milestone Dec 5, 2016
@rricard
Copy link
Contributor Author

rricard commented Dec 5, 2016

@gaearon Well, I think it's quite complete now, I heavily tested it and covered many edge cases... I'll see how we can add this to the e2e test process... and after that, I think we're good for a final review!

@rricard
Copy link
Contributor Author

rricard commented Dec 5, 2016

Well the addition to the e2e tests pass locally, just waiting for the CI!

Copy link

@gabelevi gabelevi left a comment

Choose a reason for hiding this comment

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

This is fantastic! Thanks for building this!

Small note: an empty .flowconfig file should be sufficient! I think the README is just a little out of date. Nowadays, if you run create-react-app and just do

flow init && flow check

you should get 0 errors :)

'<PROJECT_ROOT>/node_modules/fbjs/.*',
'',
'[libs]',
'./flow-typed'
Copy link

Choose a reason for hiding this comment

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

This shouldn't be necessary - Flow automatically treats the <PROJECT_ROOT>/flow-typed directory as a lib directory :)

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 ignored that, and this is pretty cool, that'll go as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arr, I still see issues as of flow 0.36.0, here's my output on a jest test:

src/App.test.js:6
  6: it('renders without crashing', () => {
     ^^ identifier `it`. Could not resolve name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, lemme retry 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, sorry, I'll have to let that for now...

Copy link
Contributor

Choose a reason for hiding this comment

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

However, modifying that file doesn't seem to trigger a rebuild.

Likely because it's not part of webpack flow (it's a test after all). So it looks like this integration is a bit limited by not working with test files. Not sure if it's easily fixable or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, yea, unfortunately, for now, we don't have any way to trigger on App.test.js alone. You have to add it toApp.js. However, once you change App.js, a rebuild will be retriggered including the test. In this case, it's random, sometimes flow-typed is correctly seen, sometimes not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gaearon By the way, linting in test files will not run as well... I think we need to find a way to run eslint+flow during the test watching... I don't exactly know just yet how to do that but I can always take a look...

Copy link
Contributor

Choose a reason for hiding this comment

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

@rricard I agree, it would be nice to figure out a way to do this. As a separate action item :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gabelevi don't ask me why, it now works every time with an empty .flowconfig

new FlowTypecheckPlugin({
flowconfig: [
'[ignore]',
'<PROJECT_ROOT>/node_modules/fbjs/.*',
Copy link

Choose a reason for hiding this comment

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

After facebook/fbjs#182 (which was in fbjs 0.8.5) this shouldn't be necessary! And from testing out create-react-app right now, it seems to install 0.8.6 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is good news, I'll get that out.


```js
node_modules/fbjs/lib/Deferred.js.flow:60
Copy link

Choose a reason for hiding this comment

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

Yeah this might be a little out of date

@gaearon
Copy link
Contributor

gaearon commented Dec 5, 2016

@gabelevi Thank you for checking in!

@gabelevi
Copy link

gabelevi commented Dec 5, 2016

😁 😁 😁 Super excited for this! 😁 😁 😁

@rricard
Copy link
Contributor Author

rricard commented Dec 5, 2016

@gaearon created an issue about those unchecked tests: #1169

@rricard
Copy link
Contributor Author

rricard commented Dec 8, 2016

@gaearon made a PR to check the tests, via a webpack plugin, once again! => #1209

@rricard rricard force-pushed the integrate-flow branch 2 times, most recently from 6fe3848 to 46829e3 Compare December 8, 2016 17:23
@SimenB
Copy link
Contributor

SimenB commented Dec 8, 2016

Thoughts on extracting all of these awesome webpack plugins into separate packages? I know I can just depend on react-dev-utils, but that seems kinda weird.

EDIT: Not related to this PR at all, of course... Can open a separate issue if you guys are interested

@gaearon
Copy link
Contributor

gaearon commented Dec 8, 2016

For now we don't want to add more packages because people will have expectations of a stable release cycle. Instead it is likely we'll need to iterate on them quite a bit before they settle down. We're also not quite ready to make them "proper" because people will start asking for configuration options, come with issues specific to those packages etc. If/when this gets really stable I'd be up for extracting it from react-dev-utils. But not very soon.

@rricard
Copy link
Contributor Author

rricard commented Apr 24, 2017

This is pretty neat! I see that this PR is in good hands!

@Timer
Copy link
Contributor

Timer commented Apr 24, 2017

@rricard we decided to chop the flow-typed integration for now, we only want integration with flow to be skin deep (that of an IDE, e.g. VSCode).

This PR should be good to go barring shutting down the flow server when the process exits (which I'm not even sure if that's a huge deal).

@rricard
Copy link
Contributor Author

rricard commented Apr 26, 2017

@Timer good call, flow-typed has always been the annoying part of this PR. I'm not sure it's a good idea to add it in a followup PR. At least not until flow-typed/flow-typed#526 is properly addressed.

@Timer
Copy link
Contributor

Timer commented Apr 27, 2017

That, and until all of definitelytyped definitions make their way in (imo).

@Timer Timer mentioned this pull request May 5, 2017
32 tasks
@gaearon
Copy link
Contributor

gaearon commented May 8, 2017

What is missing here?

@gaearon
Copy link
Contributor

gaearon commented May 8, 2017

This PR should be good to go barring shutting down the flow server when the process exits (which I'm not even sure if that's a huge deal).

I'm fine with keeping the server alive.

@Timer
Copy link
Contributor

Timer commented May 8, 2017

Only thing missing is disabling this feature for Windows users.
Also, I think it'd be nice if we explicitly use the global flow and not our local when available.

@trygveaa
Copy link
Contributor

trygveaa commented May 8, 2017

Also, I think it'd be nice if we explicitly use the global flow and not our local when available.

I'm not sure if that's a good idea. Different projects may use different versions of flow, so the global version may not be correct for the project.

This PR should be good to go barring shutting down the flow server when the process exits (which I'm not even sure if that's a huge deal).

The server might not have been started by this, and even if it was, flow might still be used by other processes, so I don't think it should be shut down.

@gaearon
Copy link
Contributor

gaearon commented May 8, 2017

Only thing missing is disabling this feature for Windows users.

Is this feature slow for Windows users or Flow itself?
We could introduce two stages:

  • Compiled successfully (Flow checks are still running)
  • Compiled successfully / with errors / whatever

@Timer
Copy link
Contributor

Timer commented May 8, 2017

@gaearon flow itself is slow on windows, but I had my VSCode's "Run flow while editing" option ticked, which made my system virtually unusable.
This might not be as exaggerated if this was disabled.

I don't want to disable it because the "feature" is slow, just because it can freeze up your system for periods of time if you have certain options turned on in your editor.

I'll have to do some more testing to see what the best path is going forward, however, I'm windows-machine-less right now.

) {
return;
}
const contents = loaderContext.fs.readFileSync(module.resource, 'utf8');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not slowing down the build? Seems suspicious to read every file if Webpack also does it by itself.

Copy link
Contributor

@Timer Timer May 10, 2017

Choose a reason for hiding this comment

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

loaderContext.fs.readFileSync is webpack's memory file system, not the real filesystem.
We're basically doing cache[key].

Copy link
Contributor

Choose a reason for hiding this comment

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

Aaaah.

@gaearon gaearon modified the milestones: 0.11.0, 0.10.0 May 15, 2017
@gaearon
Copy link
Contributor

gaearon commented May 15, 2017

After some discussion we're pushing this back to 0.11.

In particular, it doesn't seem right to wait for Flow to declare the build to be complete.
Instead, @Timer plans to investigate using the new Flow push API that Nuclide is using.

In the UI, we will probably show Flow check and Webpack compilation as two separate "panes" that don't wait for each other.

@rricard
Copy link
Contributor Author

rricard commented Jun 6, 2017

@gaearon @Timer I'm back, I'd love to sync with you if you need help on finishing that. I can particularly help to investigate the push API.

@mkozhukharenko
Copy link

any updates?

@@ -16,6 +16,7 @@ const HtmlWebpackPlugin = require('html-webpack-plugin');
const CaseSensitivePathsPlugin = require('case-sensitive-paths-webpack-plugin');
const InterpolateHtmlPlugin = require('react-dev-utils/InterpolateHtmlPlugin');
const WatchMissingNodeModulesPlugin = require('react-dev-utils/WatchMissingNodeModulesPlugin');
var FlowTypecheckPlugin = require('react-dev-utils/FlowTypecheckPlugin');

Choose a reason for hiding this comment

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

All the other imports uses const, so this not also? :)

@gaearon
Copy link
Contributor

gaearon commented Jan 9, 2018

Really sorry about this—it has gotten stale and I think we won't be pursuing this further ourselves. The Flow team is not focused on supported integrations right now so we'd always have to be playing catch-up.

That said if you're interested in reviving this on top of an API used by Nuclide, and solve the wait-before-build problem then we can revisit this.

Thanks a lot for prototyping it!

@gaearon gaearon closed this Jan 9, 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

Successfully merging this pull request may close these issues.