Fix flow nullable type definitions

[TobiasBales]

Flow uses parameter: ?type for nullable types (instead of parameter? :type as used before.
I came across the warning when checking react-native
(react-native/Libraries/CustomComponents/Navigator/Navigation/NavigationRouteStack.js:121)
and slice was passed a nullable value which resulted in errors.

[TobiasBales]

@leebyron sorry to ping you directly, it just has been a week and I figured it won't hurt ;)
Any chance for this getting in soon?

[nmn]

@TobiasBales, I've been using parameter?: type for optional parameters in my code and it works fine.

parameter?: type — The parameter is optional, but when given it must have a type.
parameter: ?type — The parameter is not optional, but is nullable
parameter?: ?type — The parameter is optional, and nullable

unless there are other changes you want to still get merged, I would suggest closing the PR.

[TobiasBales]

https://github.com/facebook/react-native/blob/master/Libraries/CustomComponents/Navigator/Navigation/NavigationRouteStack.js#L120

begin and end which are passed to Array.prototype.slice are both nullable, which results in the error.
Then maybe the better option would be to make it optional and nullable?

I will gladly amend the commit to include both

[nmn]

Nullable and optional mean different things. So it really depends on a case-by-case basis.
In the function body itself, nullable and optional result in similar type-checking concerns but it really makes difference at the call-site.

That said, the example from the React-Native code looks wrong to me. Perhaps that's a place for a PR!

As an another example let me explain what the difference is:

1. With Nullable Types:

slice(begin: ?number, end: ?number): RouteStack {
    var routeNodes = this._routeNodes.slice(begin, end);
    var index = Math.min(this._index, routeNodes.size - 1);
    return this._update(index, routeNodes);
}
/// 
slice(null, null) // legal
slice() // ERROR
slice(1, null) //legal
slice(undefined, 2) // legal
slice(2) //ERROR

1. With optional params:

slice(begin?: number, end?: number): RouteStack {
    var routeNodes = this._routeNodes.slice(begin, end);
    var index = Math.min(this._index, routeNodes.size - 1);
    return this._update(index, routeNodes);
}
/// 
slice(null, null) // ERROR
slice() // legal
slice(1, null) //ERROR
slice(undefined, 2) // ERROR
slice(2) // legal

Does that make sense?

[TobiasBales]

Yeah it does and I am aware of the distinction.
I guess I didn't think about the optional parameter aspect (when I started the pr) that was used.

My thought (after your comment) was, that slice(begin?: ?number, end?: ?number) would be the more flexible and correct (since slice does accept null values) solution.
The same can be achieved by passing 0 as first argument, so it might just not be necessary.
The question becomes if you want to build an api that is "cleaner" (by avoiding null arguments) or one that leans more toward Array.prototype etc like behaviour (by accepting null where the originals accept null) and communicating that by the used types.

[lacker]

So #878 refactored the flow types and also added tests for them. Would you mind updating this PR to merge those changes in, and also add a test that catches this? Thanks!

[leebyron]

Closing this - the changed lines are all intentionally optional function arguments and are not nullable types.