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

added initial POC for flow support #350

Closed
wants to merge 1 commit into from

Conversation

torifat
Copy link
Contributor

@torifat torifat commented Aug 4, 2016

POC for issue #324

$ npm run create-react-app my-app
$ cd my-app
$ vim .flowconfig # copy template from: https://github.com/facebookincubator/create-react-app/blob/master/template/README.md#adding-flow
$ npm start

Instead of vim .flowconfig, you can do wget https://gist.githubusercontent.com/torifat/756443d1bab62abda5f87c3ca2c90dc8/raw/ab2464634e3b0cd873d0ff302a83a5097964276f/.flowconfig too.

Sample App.js:

/* @flow */
import React, { Component } from 'react';
import logo from './logo.svg';
import './App.css';

Math.pow('1212');

class App extends Component {
  render() {
    return (
      <div className="App">
        <div className="App-header">
          <img src={logo} className="App-logo" alt="logo" />
          <h2>Welcome to React</h2>
        </div>
        <p className="App-intro">
          To get started, edit <code>src/App.js</code> and save to reload.
        </p>
      </div>
    );
  }
}

export default App;

Sample Output:
screenshot 2016-08-04 07 48 00

@ghost
Copy link

ghost commented Aug 4, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@ghost
Copy link

ghost commented Aug 4, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@ghost ghost added the CLA Signed label Aug 4, 2016
@gaearon
Copy link
Contributor

gaearon commented Aug 4, 2016

This is very nice!

aug-04-2016 13-55-29

@gaearon
Copy link
Contributor

gaearon commented Aug 4, 2016

cc @thejameskyle, I would love if someone from the Flow team could look at this and offer what can be improved in this implementation (webpack “loader” is like a browserify transform). Here’s a popular TypeScript loader for comparison.

@torifat
Copy link
Contributor Author

torifat commented Aug 4, 2016

I would love some pointers from Flow team. The current implementation in not optimal and also will hide errors if the mainFile doesn't match with the resource files. I will keep improving it throughout the week.

@kasperpeulen
Copy link
Contributor

I tried it out last couple of days, seems to work fine 👍

@gabriel-laet
Copy link

gabriel-laet commented Aug 31, 2016

@torifat @gaearon What I've noticed playing around with flowtype-loader in a larger codebase is that apparently calling flow status for each file is pretty slow. Maybe the ideal impl is call flow status once, cache and report errors for each file.

@torifat
Copy link
Contributor Author

torifat commented Aug 31, 2016

@gabriel-laet You are right. I hacked that in a night for POC. If you check the TODO section of README then you will find Fix performance issue (parse flow output once). I was in vacation and just get back today. If you want contribute you can give it a hand. Or, I will try to give it some time next week.

@ghost ghost added the CLA Signed label Aug 31, 2016
@gabriel-laet
Copy link

@torifat 👍 I will try to find some time and help on the performance issue

@ghost ghost added the CLA Signed label Aug 31, 2016
@jamiebuilds
Copy link

Hey @torifat thanks for taking the time to do this, I'd like to chat about it some more. Does Twitter DMs work for you?

@torifat
Copy link
Contributor Author

torifat commented Aug 31, 2016

@thejameskyle yes, it will do.

@ghost ghost added the CLA Signed label Aug 31, 2016
@ccorcos
Copy link

ccorcos commented Sep 10, 2016

Hmm. I'm not sure we should even use a webpack loader. Using the flow-ide package, I get errors inline in my editor as I code. Then we can just run flow before we run our test suite...

@gaearon
Copy link
Contributor

gaearon commented Sep 10, 2016

@ccorcos Editor integrations are great for sure but there’s a much wider reach when you build something in, as opposed to making it available via IDE plugins.

@ghost ghost added the CLA Signed label Sep 10, 2016
@ccorcos
Copy link

ccorcos commented Sep 11, 2016

True... What additional features would you like to see that the typescript loader offers that this flow loader doesn't?

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

I'm closing because unfortunately it doesn't appear like Flow team is interested in Create React App or Webpack integration, and there's no sense in keeping this hang here forever.

@gaearon gaearon closed this Nov 20, 2016
@torifat torifat deleted the feature/flow-support branch November 20, 2016 19:06
@jamiebuilds
Copy link

@gaearon that isn't true, I asked you to merge this over a month ago.

@jamiebuilds
Copy link

Please reopen this. In my opinion, it's ready to be merged, if you disagree let me know.

@torifat
Copy link
Contributor Author

torifat commented Nov 21, 2016

@thejameskyle need to update the implementation.

@gaearon
Copy link
Contributor

gaearon commented Nov 21, 2016

Sorry, I didn't see that message from you or maybe misunderstood.
This PR has pretty fundamental issues like torifat/flowtype-loader#4.

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants