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] Install flow-bin and use that for packager Flow check #1099

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@gabelevi
Contributor

gabelevi commented May 1, 2015

Flow is versioned with semver, and almost every deploy includes a breaking change. We can address this by installing the flow-bin package...a very simple npm package that downloads the correct binary based on the operating system and the version. Then the packager can use the correct version of Flow, even if a more recent version of Flow is available.

This is a sister PR to #1097 which also sticks the version number in the .flowconfig, so that users will get a clear error if they try to do Flow checks with the wrong version of Flow.

This means when we update react-native to work with a new version of Flow, we need to update the version number in react-native-cli/index.js and in Examples/SampleApp/_flowconfig. When a user wants to update the version of Flow they're using locally, they will just need to edit their .flowconfig and their package.json. We should document this somewhere.

Future work includes maybe transitioning people from installing flow with homebrew to globally installing some wrapper script, which can intelligently find which flow binary to use (like looking for a flow-bin package, for example).

@gabelevi

This comment has been minimized.

Show comment
Hide comment
@gabelevi

gabelevi May 1, 2015

Contributor

I tested this by doing something like

# Check out my fork of react-native
./react-native-gabelevi/react-native-cli/index.js init Foo
cd Foo
npm install ../react-native-gabelevi
./node_modules/react-native/init.sh Foo
  • I tried installing the current version of Flow (0.10.0) and an old version of Flow (0.9.2) to make sure it would install the right package.
  • Made sure that npm install --save uses the ^ to specify the version, and that should mean exactly that version plus or minus patches, since the version number is < 1.0.0.
  • I messed around with the .flowconfig, and made sure the red box showed up when the .flowconfig's version didn't match the flow-bin version.
  • I added some flow errors and made sure the red box showed up.
  • I nuked the node_modules/flow-bin package and made sure the packager fell back to flow (whatever is on the path)
Contributor

gabelevi commented May 1, 2015

I tested this by doing something like

# Check out my fork of react-native
./react-native-gabelevi/react-native-cli/index.js init Foo
cd Foo
npm install ../react-native-gabelevi
./node_modules/react-native/init.sh Foo
  • I tried installing the current version of Flow (0.10.0) and an old version of Flow (0.9.2) to make sure it would install the right package.
  • Made sure that npm install --save uses the ^ to specify the version, and that should mean exactly that version plus or minus patches, since the version number is < 1.0.0.
  • I messed around with the .flowconfig, and made sure the red box showed up when the .flowconfig's version didn't match the flow-bin version.
  • I added some flow errors and made sure the red box showed up.
  • I nuked the node_modules/flow-bin package and made sure the packager fell back to flow (whatever is on the path)
function doFlowTypecheck(res, flowroot, next) {
var flowCmd = 'cd "' + flowroot + '" && flow --json --timeout 20';
function doFlowTypecheck(res, flowbin, flowroot, next) {
var flowCmd = 'cd "' + flowroot + '" && ' + flowbin + ' --json --timeout 20';

This comment has been minimized.

@vjeux

vjeux May 13, 2015

Contributor

please escape all those commands

@vjeux

vjeux May 13, 2015

Contributor

please escape all those commands

This comment has been minimized.

@ide

ide Jul 30, 2015

Collaborator

A good way is to use:

execFile(flowbin, ['--json', '--timeout', 20], {cwd: flowroot}, (flowError, stdout, stderr) => {

});
@ide

ide Jul 30, 2015

Collaborator

A good way is to use:

execFile(flowbin, ['--json', '--timeout', 20], {cwd: flowroot}, (flowError, stdout, stderr) => {

});
@@ -15,6 +15,9 @@
.*/node_modules/react-tools/src/core/ReactInstanceHandles.js
.*/node_modules/react-tools/src/event/EventPropagators.js
# Ignore flow-bin
.*/node_modules/flow-bin/.*

This comment has been minimized.

@vjeux

vjeux May 13, 2015

Contributor

flow-bin doesn't type check, how ironic

@vjeux

vjeux May 13, 2015

Contributor

flow-bin doesn't type check, how ironic

This comment has been minimized.

@gabelevi

gabelevi May 13, 2015

Contributor

We currently package the Flow binary with some examples. Maybe we should stop doing that...

@gabelevi

gabelevi May 13, 2015

Contributor

We currently package the Flow binary with some examples. Maybe we should stop doing that...

@brentvatne brentvatne changed the title from Install flow-bin and use that for packager Flow check to [Flow] Install flow-bin and use that for packager Flow check Jun 1, 2015

@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Jun 11, 2015

Contributor

@gabelevi: what's your plan here? Tests failed so at least needs a rebase, or just close it out if it's out of date.

Contributor

sahrens commented Jun 11, 2015

@gabelevi: what's your plan here? Tests failed so at least needs a rebase, or just close it out if it's out of date.

@gabelevi

This comment has been minimized.

Show comment
Hide comment
@gabelevi

gabelevi Jun 12, 2015

Contributor

@sahrens - I do still think this is a good idea. If I rebase & get the tests passing would you take this PR?

Contributor

gabelevi commented Jun 12, 2015

@sahrens - I do still think this is a good idea. If I rebase & get the tests passing would you take this PR?

@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Jun 12, 2015

Contributor

Yeah sure.
On Thu, Jun 11, 2015 at 7:26 PM Gabe Levi notifications@github.com wrote:

@sahrens https://github.com/sahrens - I do still think this is a good
idea. If I rebase & get the tests passing would you take this PR?


Reply to this email directly or view it on GitHub
#1099 (comment)
.

Contributor

sahrens commented Jun 12, 2015

Yeah sure.
On Thu, Jun 11, 2015 at 7:26 PM Gabe Levi notifications@github.com wrote:

@sahrens https://github.com/sahrens - I do still think this is a good
idea. If I rebase & get the tests passing would you take this PR?


Reply to this email directly or view it on GitHub
#1099 (comment)
.

@@ -48,6 +48,7 @@
"chalk": "^1.0.0",
"connect": "2.8.3",
"debug": "~2.1.0",
"flow-bin": "^0.11.0",

This comment has been minimized.

@ide

ide Jun 30, 2015

Collaborator

This should be "0.11.0" (or I guess 0.13.1 now) without the caret since .flowconfig's [version] doesn't support semver yet (I think?). Opened up facebook/flow#592.

@ide

ide Jun 30, 2015

Collaborator

This should be "0.11.0" (or I guess 0.13.1 now) without the caret since .flowconfig's [version] doesn't support semver yet (I think?). Opened up facebook/flow#592.

This comment has been minimized.

@ide

ide Jul 30, 2015

Collaborator

there is now caret support so this can safely be ^0.14.0!

@ide

ide Jul 30, 2015

Collaborator

there is now caret support so this can safely be ^0.14.0!

flowbin = 'flow';
}
exec('command -v ' + flowbin + ' >/dev/null 2>&1', function(error, stdout) {

This comment has been minimized.

@ide

ide Jul 30, 2015

Collaborator

This should work and be safe I think:

execFile('command', ['-v', flowbin], error => {
});
@ide

ide Jul 30, 2015

Collaborator

This should work and be safe I think:

execFile('command', ['-v', flowbin], error => {
});
@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Jul 30, 2015

Collaborator

@gabelevi can you rebase this and squash your commits since flow 0.14 is out and will be getting automatically installed by the react native tests?

Collaborator

ide commented Jul 30, 2015

@gabelevi can you rebase this and squash your commits since flow 0.14 is out and will be getting automatically installed by the react native tests?

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Dec 22, 2015

Collaborator

@gabelevi Any updates on this?

Collaborator

satya164 commented Dec 22, 2015

@gabelevi Any updates on this?

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 22, 2015

@gabelevi updated the pull request.

facebook-github-bot commented Dec 22, 2015

@gabelevi updated the pull request.

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Jan 3, 2016

Collaborator

I think this is a good idea too... looking forward to getting it in @gabelevi!

Collaborator

brentvatne commented Jan 3, 2016

I think this is a good idea too... looking forward to getting it in @gabelevi!

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Feb 29, 2016

@gabelevi do you have any updates for this pull request? It's been a while so we wanted to check in and see if you've looked at the requested changes.

ghost commented Feb 29, 2016

@gabelevi do you have any updates for this pull request? It's been a while so we wanted to check in and see if you've looked at the requested changes.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Mar 20, 2016

Contributor

Hey @gabelevi! Thanks for making the pull request, but we are closing it due to inactivity (20 days with no activity). If you want to get your proposed changes merged, please rebase your branch with master and send a new pull request. :)

Contributor

mkonicek commented Mar 20, 2016

Hey @gabelevi! Thanks for making the pull request, but we are closing it due to inactivity (20 days with no activity). If you want to get your proposed changes merged, please rebase your branch with master and send a new pull request. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment