mapN should narrow return type to remove null
Closed this issue · 5 comments
Given the following code with strictNullChecks enabled:
const a: Option<string> = Option.of(1).mapN(x => 'foo' as string | null);I get this error:
Type 'Option<string | null>' is not assignable to type 'Option<string>'.
Type 'string | null' is not assignable to type 'string'.
Type 'null' is not assignable to type 'string'.
This is because a has type Option<string | null>. I would expect mapN to return Option<string>.
We can fix this by updating the mapN type definition to:
mapN<B>(f: (a: A) => B | null): Option<B>;What do you think? Perhaps we should also include undefined in the return type of f.
I don't understand that error, but if that's what it takes, then fine.
@alexandru I'm happy to raise a PR if you agree this is the correct change.
I'm not familiar with Flow, so I may need some help updating those annotations.
@OliverJAsh feel free to work on a PR, I'll be happy to help.
There isn't much to Flow annotations, besides variance annotations, they are pretty much similar to .d.ts files, with some minor syntactic differences. And in order to see that you're correct, you only need to run flow check within packages/funfix-core.
Anyway, in case you need to also update the Flow annotations, then the file you need to modify is this: https://github.com/funfix/funfix/blob/master/packages/funfix-core/src/disjunctions.js.flow
However, we don't know if Flow needs the same treatment, so the first thing you need to do is to add a "Flow test" here: https://github.com/funfix/funfix/blob/master/packages/funfix-core/test/flow/option.test.js.flow
(which you then check with flow check or with yarn test:prod)
These Flow tests are only compile-time tests, checking to see if the compiler can compile the source files, along with the tests, or not. They are never executed, so you can add whatever code you want, the purpose being to see if the compiler agrees with you.
How does #70 look?
Looking good, merging tomorrow.