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

charAt, charCodeAt and codePointAt are weird #8284

Open
nnmrts opened this issue Feb 4, 2020 · 1 comment · May be fixed by #8474
Open

charAt, charCodeAt and codePointAt are weird #8284

nnmrts opened this issue Feb 4, 2020 · 1 comment · May be fixed by #8474
Labels
Library definitions Issues or pull requests about core library definitions

Comments

@nnmrts
Copy link
Contributor

nnmrts commented Feb 4, 2020

Flow version: 0.117.1

Expected behavior

const string = "ABC";

string.charAt();
string.charCodeAt();
string.codePointAt();
// ^ no errors

const maybeANumber = string.codePointAt(100);

Math.round(maybeANumber);
// ^ errors because maybeANumber could be undefined

if (typeof maybeANumber === "number") {
	Math.round(maybeANumber);
	// ^ should be fine now because we checked it this time
}

Actual behavior

const string = "ABC";

string.charAt();
string.charCodeAt();
string.codePointAt();
// ^ errors "Cannot call `string.[...]` because  function [1] requires another argument."

const maybeANumber = string.codePointAt(100);

Math.round(maybeANumber);
// ^ doesn't error but maybeANumber could be undefined
  • Link to Try-Flow or Github repo:
    here

Explanation

In my opinion, the definitions of these methods are kind of wrong, because

  1. they don't actually require an argument; passing no argument means NaN which means 0 in this context
  2. codePointAt could return undefined when pos is greater than the string's length (ref)
  3. there's no reason for the charAt parameter being called pos instead of index while the others are not (ref)
    • MDN only calls the codePointAt parameter pos (ref)
    • TC39 calls all of them pos, but also "index" in their explanations (ref)
    • we developers probably would all call them index

Point 1 is debatable though, because ECMAScript actually specifies this:
String.prototype.charCodeAt ( pos )
and not
String.prototype.charCodeAt ( [ pos ] )
which they normally do for actually optional parameters, but in this case, pos will always get converted to NaN if it's undefined anyways. And passing NaN to those methods always returns the first index.
BUT, for example the flow definition of the global function parseInt is technically also not following ECMAScript. Flow says that parseInt accepts mixed as string, but it doesn't. And it says that radix is optional, but it actually isn't. Well, it won't throw, but almost every function of core JavaScript won't throw because of type-juggling and intelligent implementation. I get why Flow allows mixed here, otherwise we would have a pretty hard time checking our strings.
Same goes for isNaN for example. Would be weird if Flow errors on isNaN("test") just because we wanted to check that value, right?
In my opinion, string.charCodeAt() to get the first index should be allowed. It's a common pattern.

Point 2 should be pretty clear.

Point 3 is not that important but I think we should decide on one parameter name for all three methods here.

So my proposal is that this:

	// ...
	charAt(pos: number): string;
    charCodeAt(index: number): number;
    codePointAt(index: number): number;
	// ...

should be this:

	// ...
	charAt(index?: number): string;
    charCodeAt(index?: number): number;
    codePointAt(index?: number): number | undefined;
	// ...

or maybe just this:

	// ...
	charAt(index: number): string;
    charCodeAt(index: number): number;
    codePointAt(index: number): number | undefined;
	// ...

Thoughts?

@jamesisaac jamesisaac added Library definitions Issues or pull requests about core library definitions and removed bug needs triage labels Feb 5, 2020
@Brianzchen
Copy link
Contributor

I like this. I think it adds more protection to a codebase to prevent runtime errors. The same thing happens with 'string'[123] which is defined as always returning a string but should be either string | void which I've seen a few complaints about this before.

For being optional params, I think the current impl is fine because the function is quite useless if you pass nothing in. This can help to ensure you're writing useful code.

Thoughts @nnmrts ?

I'll raise a PR and see if it sticks.

@Brianzchen Brianzchen linked a pull request Aug 30, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Library definitions Issues or pull requests about core library definitions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants