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

Disable coersion of addition of number and string. #4610

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@weakish
Copy link

weakish commented Aug 16, 2017

Now flow will report type error on code like 1 + '%'.

resolve #2303


@nmote

This comment has been minimized.

Copy link
Contributor

nmote commented Jan 8, 2019

This seems like a good idea to me, but it will likely lead to a tough rollout on large codebases. I'm not sure if there would be consensus among the team.

If you are still interested, could you rebase and make sure that tests are passing? If you do, I'll bring this up with other team members to see what they think.

Thanks for the contribution, and sorry for the long wait.

@TrySound

This comment has been minimized.

Copy link
Collaborator

TrySound commented Jan 8, 2019

Or maybe we should go with ocaml way and leave this unchanged? More safety and unexpected string conversions which are common problem in javascript.

I work with JS for 5 years and still not sure what is the result of '5' + 5 :)

@nmote

This comment has been minimized.

Copy link
Contributor

nmote commented Jan 8, 2019

@TrySound, this PR disables implicit coercions. I think we are in agreement here.

@TrySound

This comment has been minimized.

Copy link
Collaborator

TrySound commented Jan 8, 2019

Ah, sorry. Misread. I really like it then! Thanks

Disable coersion of addition of number and string.
Now flow will report type error on code like `1 + '%'`.

resolve #2303

@weakish weakish force-pushed the weakish:disbale-coersion-of-number-string-addition branch from 0645476 to fd6f580 Jan 9, 2019

@weakish

This comment has been minimized.

Copy link
Author

weakish commented Jan 10, 2019

could you rebase and make sure that tests are passing?

Rebased.

@facebook-github-bot
Copy link

facebook-github-bot left a comment

@nmote has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@nmote

This comment has been minimized.

Copy link
Contributor

nmote commented Jan 10, 2019

I started a discussion with others on the team about this. While we agree that this is a good idea in principle, we are concerned about the rollout pain (this causes 16,000 new errors in one of our repositories) and the frustration that it could cause users. Of course, the improved safety is a worthwhile goal but we don't think the tradeoff is worth it at this time.

We are interested in making this a Flow lint rule, though. That way it could be configured as a warning or error (or left off altogether) on a per-project basis. I'm not very familiar with how lint rules are implemented but I could spend some time on it if you are interested in reimplementing this as a lint rule.

Feel free to CC me in an issue if you would like implementation guidance.

@nmote nmote closed this Jan 10, 2019

@weakish

This comment has been minimized.

Copy link
Author

weakish commented Jan 13, 2019

We are interested in making this a Flow lint rule, though.

Great. Should this be part of the sketchy-number rule or a seperated rule?

@nmote

This comment has been minimized.

Copy link
Contributor

nmote commented Jan 24, 2019

@weakish, sorry, your response slipped through the cracks. This should be a separate lint rule. Could we move any future design discussion to an issue? Feel free to CC me when you open it.

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