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

Support flow experimental.const_params=true #16228

Closed
ckknight opened this issue Oct 5, 2017 · 11 comments
Closed

Support flow experimental.const_params=true #16228

ckknight opened this issue Oct 5, 2017 · 11 comments
Labels
Bug Flow Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. JavaScript Resolution: Locked This issue was locked by the bot.

Comments

@ckknight
Copy link

ckknight commented Oct 5, 2017

Is this a bug report?

Yes

Have you read the Contributing Guidelines?

Yes

Environment

Environment:
OS: macOS High Sierra 10.13
Node: 6.11.2
Yarn: 1.1.0
npm: 3.10.10
Watchman: 4.7.0
Xcode: Xcode 9.0 Build version 9A235
Android Studio: Not Found

Packages: (wanted => installed)
react: 16.0.0-alpha.12 => 16.0.0-alpha.12
react-native: ^0.48.4 => 0.48.4

Steps to Reproduce

(Write your steps here:)

  1. Create a new react-native app with create-react-native-app
  2. yarn add --dev flow-bin@^0.49.1 (same version specified in .flowconfig)
  3. Add experimental.const_params=true under [options] in .flowconfig
  4. ./node_modules/.bin/flow

Expected Behavior

No errors.

(Write what you thought would happen.)

Actual Behavior

$ flow
node_modules/fbjs/lib/UnicodeBidi.js.flow:90
 90:   fallback = fallback || UnicodeBidiDirection.NEUTRAL;
       ^^^^^^^^ fallback. const param cannot be reassigned (see experimental.const_params=true in .flowconfig)
 89: function resolveBlockDir(str: string, fallback: ?BidiDirection): BidiDirection {
                                           ^^^^^^^^^^^^^^^^^^^^^^^^ const param fallback

node_modules/fbjs/lib/UnicodeBidi.js.flow:92
 92:     return fallback;
                ^^^^^^^^ null. This type is incompatible with the expected return type of
 89: function resolveBlockDir(str: string, fallback: ?BidiDirection): BidiDirection {
                                                                      ^^^^^^^^^^^^^ string enum

…[removed for brevity]…

... 88 more errors (only 50 out of 138 errors displayed)
To see all errors, re-run Flow with --show-all-errors

Reproducible Demo

https://github.com/ckknight/ReactNative-const_params-repro

@ckknight
Copy link
Author

ckknight commented Oct 6, 2017

This also affects react-native@0.49.0

@janicduplessis
Copy link
Contributor

Some of there errors seem to come from RN dependencies (fbjs from the sample you provided). I think it would be nice to fix but will require changing stuff across repos. If someone want to send a PR to fix some errors that are in this repo it would be nice.

@janicduplessis janicduplessis added Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. JavaScript labels Oct 13, 2017
@rajsek
Copy link

rajsek commented Nov 6, 2017

Hi @janicduplessis: I am a beginner in opensource contribution and like to work on this.
Could you please guide me to fix this issue?

@TakaGoto
Copy link

TakaGoto commented Feb 5, 2018

@janicduplessis would like to work on this since it seems like an easy one for first timer. just to clarify, this would be to add experimental.const_params=true to react-native's .flowconfig and find/fix all the ones it outputs.

@klvs
Copy link
Contributor

klvs commented Feb 20, 2018

I have a PR inbound for this. Should be submited today or tomorrow.

@klvs
Copy link
Contributor

klvs commented Feb 20, 2018

@TakaGoto I'll tag you in my PR in case you'd like to submit any changes as well. You need to clone this repo, enable experimental.const_params and run flow. Most of the errors are fixed with a minor refactor.

@elicwhite
Copy link
Member

elicwhite commented Feb 23, 2018

I really really want to do this. Unfortunately, we have a monorepo at facebook and everything is under a single flow config. If we merge this, it wouldn't change how flow treats these files internally and we'd be likely to break flow for everyone in open source very soon after this gets merged.

Fortunately, I actually wrote a pretty intensive codemod for this at Facebook so we could convert our entire codebase and turn this flow config on. Unfortunately, there is a remaining issue with the codemod that has kept me from converting our entire codebase. The codemod erroneously converts:

function foo(myVar) {
  var myVar = 2; // valid shadowing of the function argument
}

into

function foo(myVar) {
  let localMyVar = myVar;
  var localMyVar = 2; // Uncaught SyntaxError: Identifier 'localMyVar' has already been declared
}

If you would be interested in detecting this scope issue and getting the codemod to handle it, we'd be able (and happy) to make this change to react native.

A PR (with tests) with our codemod from Facebook is here: cpojer/js-codemod#85

@klvs
Copy link
Contributor

klvs commented Feb 23, 2018

Very interesting. I'll take a look, thanks!

@react-native-bot
Copy link
Collaborator

Thanks for posting this! It looks like you may not be using the latest version of React Native, v0.53.0, released on January 2018. Can you make sure this issue can still be reproduced in the latest version?

I am going to close this, but please feel free to open a new issue if you are able to confirm that this is still a problem in v0.53.0 or newer.

How to ContributeWhat to Expect from Maintainers

@react-native-bot react-native-bot added Ran Commands One of our bots successfully processed a command. Stale There has been a lack of activity on this issue and it may be closed soon. and removed Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. JavaScript labels Feb 24, 2018
@stale stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 24, 2018
@elicwhite
Copy link
Member

Silly bot

@elicwhite elicwhite reopened this Feb 24, 2018
@elicwhite elicwhite added Good first issue Interested in collaborating? Take a stab at fixing one of these issues. JavaScript Help Wanted :octocat: Issues ideal for external contributors. and removed Ran Commands One of our bots successfully processed a command. labels Feb 24, 2018
@elicwhite elicwhite changed the title Errors with flow experimental.const_params=true Support flow experimental.const_params=true Oct 21, 2018
@hramos hramos added the Flow label Dec 14, 2018
@hramos hramos removed the Bug Report label Feb 6, 2019
@kelset
Copy link
Contributor

kelset commented Mar 19, 2019

👋 there - this issue is still technically "not fixed", for the reason Eli explained above.

Basically every instance similar to this in the main codebase

function foo(arg) {
  arg += 2;
  console.log(arg);
}

would need to be changed to

function foo(arg) {
  let localArg = arg;
  localArg += 2;
  console.log(localArg);
}

There is a codemod Eli worked on -> cpojer/js-codemod#85 which would help a lot in automating this process, but there is a 1% cases in which it would conflict.

So, all in all, there is a lot of work related to "code hygiene" - we'd love to see some PRs to help with this effort but it's really low prior right now. So I'm going to close it as there are more important tasks to focus on.

@kelset kelset closed this as completed Mar 19, 2019
@facebook facebook locked as resolved and limited conversation to collaborators Mar 19, 2020
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Mar 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Flow Good first issue Interested in collaborating? Take a stab at fixing one of these issues. Help Wanted :octocat: Issues ideal for external contributors. JavaScript Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

9 participants