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

[PR] Return union void for some string functions #8474

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Brianzchen
Copy link
Contributor

Closes #8284

codePointAt and ''[123] has the potential to return undefined. This change will enforce users to put guards around these use cases to provide safer string manipulation.

@Brianzchen
Copy link
Contributor Author

For my index test I had to write it as (new String()[123]: string); because (''[123]: string) wasn't throwing an error. Not sure if I'm missing something here because the previous test that uses string literal works fine.

@Brianzchen Brianzchen added the Library definitions Issues or pull requests about core library definitions label Aug 30, 2020
@nnmrts
Copy link
Contributor

nnmrts commented Nov 9, 2020

@Brianzchen
Looks good, also thanks for the parameter name changes.

I don't really care about flow anymore these days though, sorry.

@Brianzchen
Copy link
Contributor Author

I've removed the index of check to make this PR a bit easier to get in. Flow team has already expressed that is an area of unsoundness that they are prepared to allow for an easier dev experience.

@nnmrts I understand, hope things get better and you feel interested again soon 😄

Copy link
Contributor

@nnmrts nnmrts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the last commits but I don't know if this does anything or is even relevant anymore.

@Brianzchen
Copy link
Contributor Author

I think so, the definition if it hasn't been corrected is in fact wrong.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/codePointAt#return_value

It's documented as

If index is out of the range of 0 – str.length - 1, codePointAt() returns undefined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Library definitions Issues or pull requests about core library definitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

charAt, charCodeAt and codePointAt are weird
3 participants