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

Allow mixed args to String.prototype.concat() #8729

Closed
wants to merge 1 commit into from

Conversation

justingrant
Copy link

Fixes #8728 and unblocks facebook/react#22064.

According to the MDN docs for the concat() method of the String built-in type:

If the arguments are not of the type string, they are converted to string values before concatenating.

Passing numbers, objects, etc to String.prototype.concat is is valid JS behavior but causes an error in Flow. It's also blocking facebook/react#22064.

This PR changes the arg type from Array<string> to Array<mixed>.

@justingrant
Copy link
Author

BTW, I'm happy to fix the test failures and to add more tests, but before digging into that work I'd like to get some guidance from maintainers about whether this PR is welcome or not.

According to MDN docs for the `concat()` method of the `String` type:

> If the arguments are not of the type string, they are converted to string
> values before concatenating.

Passing numbers, objects, etc to `String.prototype.concat` is is valid JS
behavior but currently causes an error in Flow. It also may be blocking
facebook/react#22064.

This commit changes the arg type: `Array<string>` -> `Array<mixed>`.
@samwgoldman
Copy link
Member

Hi, Justin. I left a comment on the linked React PR, since the context around that change serves as motivation for this one, and just to keep the dialog in one spot.

@justingrant
Copy link
Author

Looks like this PR isn't aligned with Flow's goals around avoiding implicit type casts, so I'll close this PR to keep the noise of everyone's plate.

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

Successfully merging this pull request may close these issues.

String.prototype.concat() should accept Array<mixed>, not only Array<string>
3 participants