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

Array#includes check with enum #4246

Open
deecewan opened this issue Jun 26, 2017 · 14 comments
Open

Array#includes check with enum #4246

deecewan opened this issue Jun 26, 2017 · 14 comments

Comments

@deecewan
Copy link
Contributor

Hey,

So, I'm pretty sure that this will have a fairly logical solution, but I've been trawling through bugs and stack overflow and, I can't find a) a solution, and more importantly b) an explanantion.

I'm trying to validate that an item is part of an enum.

type StringEnum = 'first_option' | 'second option' | 'third option'

const options: Array<StringEnum> = ['first_option', 'second option', 'third option']

function checkInEnum(item: string): boolean {
     if (options.includes(item)) {
        console.log('it\'s a valid option')
        return true;
     }
     return false;
}

checkInEnum('some item')
checkInEnum('first_option')

However, in the includes call, flow gets mad because item is not a valid subset of StringEnum. Which I (probably) know, and I'm trying to check for.

Is there a nice way to fix this?

Flow Try

@niieani
Copy link

niieani commented Jun 26, 2017

Use: checkInEnum(item: StringEnum) instead. This way you'll catch incorrect usage of checkInEnum build-time, rather than in runtime. This kinda makes the function not necessary.

Why would you want to do the check otherwise? Are you receiving the value from a backend?

@deecewan
Copy link
Contributor Author

that was actually a terrible example, sorry.

a better function signature would be:

function normalizeValue(item: string) {
  if (options.includes(item)) {
    return item;
  }
  return item.split(' ').join(',');
}

I need to do different things based on whether the value is in the array or not, which is why I need to check. The value is 'kind of' user provided. If it's in the list, it's been set using a toggle, otherwise, it's been set using a text field.

@niieani
Copy link

niieani commented Jun 26, 2017

Ok, that makes sense. Not sure how to type this without casting item to any within the .includes call. In TypeScript you'd simply cast item as StringEnum in the call to includes, but in Flow that's not possible.

It would be nice to have a clean solution here. It's not only includes but a range of similar typing issues.

@madbence
Copy link

this sounds like a bug to me (?), .include is defined on $ReadOnlyArray<T>, so it should accept anything that is a supertype of T.

@popham
Copy link
Contributor

popham commented Jun 26, 2017

It feels like a tautological "explanation," but this doesn't work because string is not a subtype of your enum and the signature of includes looks like so:

includes(searchElement: T, fromIndex?: number): boolean;

By the spec this is too restrictive: http://www.ecma-international.org/ecma-262/7.0/#sec-array.prototype.includes. That said, I prefer it this way because I'm certain that I can conjure up use cases that benefit from it as-is (and I prefer false negatives to false positives).

Casting is the quick work around:

function normalizeValue(item: string) {
  if (((options:any):Array<string>).includes(item)) {
    return item;
  }
  return item.split(' ').join(',');
}

I like the following:

type StringEnum = 'first_option' | 'second option' | 'third option'
const options = ['first_option', 'second option', 'third option'];

// Assert that `options` satisfies your old type, but don't impose the type on `options`
(options:Array<StringEnum>);

function normalizeValue(item: string) {
  if ((options:Array<string>).includes(item)) {
    return item;
  }
  return item.split(' ').join(',');
}

@ryanscottaudio
Copy link

ryanscottaudio commented Oct 2, 2017

@popham how would you fix something like https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVAXAngBwKZgDKGATgJYB2A5gKIUCuAtmALxgDkUZJAzhgPpwcGMnArswAHw488AYzEATQcNHipHDAAtuyoSLHsA3KgUU+YfWp6swAbU7c+Kg+IA0M+Updr2H9tq6PoYAuiaoUPQUcq5gFHAkjACGMGQAXngAain0eAAUZBh4jABcYHzk1ACUZcSVtAzMAN6oYGAkeBj0JBSWqmI8AHSUcjD0ing8BUWMVa1tYAD8YIXF821lVgN2AAxhqAC+QA?

type StringEnum = 'first_option' | 'second_option' | 'third_option';
const options = ['first_option', 'second_option', 'third_option'];

function normalizeValue(item: string): StringEnum {
  return options.includes(item)
    ? item
    : options[0];
}

Seems to me that this should work with or without any sort of casting.

@niieani
Copy link

niieani commented Oct 2, 2017

@ryanscottaudio
Copy link

ryanscottaudio commented Oct 2, 2017

@niieani thanks for the workaround. what if it was a $ReadOnlyArray?

also would there be a way to do it WITH casting? I couldn't figure one out.

@niieani
Copy link

niieani commented Oct 2, 2017

@ryanscottaudio yeah, that should work. All of the $ReadOnlyArray methods could be typed much more narrowly, including includes(x).

@ryanscottaudio
Copy link

ryanscottaudio commented Oct 2, 2017

@niieani it doesn't work with $ReadOnlyArray: https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVAXAngBwKZgDKGATgJYB2A5gKIUCuAtmALxgDkUZJAzhgPpwcGMnArswAHw488AYzEATQcNHipHDAAtuyoSLHsA3KgUU+YfWp4AuMABIASngCGigPIUYWAIIkSLlgAPMTk1HRMAHysYADanNx8KgbiADQy8krJauzp7Nq62YYAuiaoivIwLiQEUPQUcilglHIw9BU8ABRkGHiMdnxhVACUdgBGcHAwrhRgAKRyWvIA1jxgnahglqpiPAB0LW0d3b2Mw6jnqHUNTRRwJIwuMGQAXngAak-0eCd9A6SUKgaUKAiJnOwg8IMZgAb02YBqGHoJFmh3aeC6PT65y2WwA-M1TvCtnYrLtYgAGUqoAC+QA. I also don't get why your workaround works; what if options wasn't read-only and someone did options.unshift('not_part_of_string_enum')? Then options[0] wouldn't satisfy StringEnum, which is what you were talking about with Flow not being able to statically infer the contents of options, right?

@niieani
Copy link

niieani commented Oct 2, 2017

@ryanscottaudio yes, I know it doesn't work with $ReadOnlyArray. That's why I wrote it "should work", not that it works 😆. Something that could be improved with Flow. Workaround works because you explicitly declare that the declared include function can verify the input is only one of the 3 strings, nothing else.

@ryanscottaudio
Copy link

@niieani hahaha sorry, i thought you meant it should work if you put it in there. for the workaround though, options[0] wouldn't be a StringEnum if you unshifted something else into options, and that has nothing to do with includes; normalizeValue is supposed to return a StringEnum regardless of if includes returns true or false, and if options[0] isn't a StringEnum because something got unshifted into it beforehand, then normalizeValue won't return a StringEnum if includes(item) returns false

@Hypnosphi
Copy link
Contributor

I ran into exactly the same issue as in #4246 (comment)
This suggestion to use .find turned out to be quite useful:
#2982 (comment)

https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVAXAngBwKZgDKGATgJYB2A5gKIUCuAtmALxgDkUZJAzhgPpwcGMnArswAHw488AYzEATQcNHipHDAAtuyoSLHsA3KgUU+YfWp6swAbU7c+Kg+IA0M+Updr2H9tq6PoYAuiaoUPQUcq5gFHAkjACGMGQAXngAain0eAAUZBh4jABcYHzk1ACUZcSVtAzMAN6oYGAkeBj0JBSWqmI8AHRcFIp5VmKsAHx9sSzzYIXFVVLSE+Z2AAxhqAC+QA

@noppa
Copy link
Contributor

noppa commented Aug 4, 2020

The type definition of Array.includes has been changed at some point and OP's code no longer errors. This issue can be closed.

FWIW I think the original behaviour was better and the current type definition allows some easy to make mistakes to go unnoticed, as demonstrated in a similar TypeScript issue, but I guess that's a subject for another time and another issue.

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

No branches or pull requests

7 participants