Skip to content
This repository has been archived by the owner on Jan 18, 2022. It is now read-only.

Added TypeScript definitions file #17

Merged
merged 2 commits into from Apr 9, 2018
Merged

Added TypeScript definitions file #17

merged 2 commits into from Apr 9, 2018

Conversation

vyshkant
Copy link
Contributor

@vyshkant vyshkant commented Mar 12, 2018

Here are some issues to be discussed:

  1. File src/types.js exports Navigator type:
// @flow

import type { NavigationContainer } from 'react-navigation';

export type Navigator = NavigationContainer<*, *, *>;

I tried to just give it a TS-style, but found that NavigationContainer of react-navigation is not generic (see https://github.com/DefinitelyTyped/DefinitelyTyped/blob/9bb6dc2f8bc00c10f8ce8af74916051c84565ffc/types/react-navigation/index.d.ts#L559-L566)

So I'm not sure if this line is correct:

export type Navigator = NavigationContainer<any, any, any>;
  1. A similar problem with the file src/reducer.js that exports function createNavigationReducer:
// @flow

import type { Reducer } from 'redux';
import type { Navigator } from './types'

function createNavigationReducer(navigator: Navigator): Reducer<*, *> {
    // ...
}

The problem is that the latest release of redux has definition of Reducer that contains only one generic argument (see https://github.com/reactjs/redux/blob/8f60ba321e8ba5fa71d60fa35573c2cdf9c0d852/index.d.ts#L57)
(beta pre-releases has two generic arguments, but pre-release is not a release)

So I'm not sure if this line is correct:

export function createNavigationReducer(navigator: Navigator): Reducer<any, any>;

index.d.ts file was missing
Closes #4
@vyshkant vyshkant mentioned this pull request Mar 12, 2018
@Ashoat
Copy link
Member

Ashoat commented Mar 12, 2018

Thanks @vyshkant for the PR! I'm not very familiar with TypeScript so I will defer to you and the community on what the types should be. Please let me know if and when the issues are resolved and you think the PR is ready to be merged.

@rodrigoelp
Copy link
Contributor

Hi @vyshkant, I have checked your code and I can see the issues you are mentioning.

  1. As specified in the DefinitelyTyped repo, the NavigationContainer is not generic, yet I can see in react-navigation's repo the type has been defined to be generic. I think this is a bug in the type definition. Probably out of date.

  2. The way to resolve this issue is to create similar versions as redux, then edit the typedef to match the type we are expecting. Or just support whatever is released and be on the watch out for the update. As it stands right now, you shouldn't use Reducer<S, A> as it has not been released. (IMHO)

@vyshkant
Copy link
Contributor Author

@rodrigoelp Based on your answer, I can draw the following conclusions:

  1. DefinitelyTyped repo often lags behind the actual development of projects, so the definitions within the repo can be outdated. Nevertheless, IMO, we can not rely on the NavigationContainer definition with generics, because the user cannot obtain this generic definition. In other words, before solving the problems on DefinitelyTyped (@types/react-navigation Incorrect type for NavigationContainer DefinitelyTyped/DefinitelyTyped#24246 and, it seems, [@types/react-navigation] NavigationContainer not generic DefinitelyTyped/DefinitelyTyped#22604) we will be forced to use non-generic definition of NavigationContainer (if you do not mind, I will make changes to the PR).

  2. I fully agree with your conclusions (and will make changes to the PR).

@rodrigoelp
Copy link
Contributor

Yup, I am good with that.

@vyshkant
Copy link
Contributor Author

And I'm also going to replace the any type with ReducerState type in the definition of createNavigationReducer function:

export function createNavigationReducer(navigator: Navigator): Reducer<ReducerState>;

because it's implementation actually use ReducerState type:

// src/reducer.js
function createNavigationReducer(navigator: Navigator): Reducer<*, *> {
  const initialState = navigator.router.getStateForAction(initAction, null);
  return (
    state: ReducerState = initialState,
    action: NavigationAction,
  ): ReducerState => {
    return navigator.router.getStateForAction(action, state);
  };
};

Some of definitions in the index.d.ts file were incorrect.
Closes #4
@vyshkant
Copy link
Contributor Author

@Ashoat I think the PR is ready to be merged.

@vyshkant
Copy link
Contributor Author

@Ashoat could you please take a look and merge this PR?

@Ashoat
Copy link
Member

Ashoat commented Apr 9, 2018

My apologies, I'm not sure how I missed this for a whole month.

@Ashoat Ashoat merged commit 8744086 into react-navigation:master Apr 9, 2018
@Ashoat
Copy link
Member

Ashoat commented Apr 9, 2018

I just published react-navigation-redux-helpers@1.0.4, which includes these definitions. Thanks for the PR, and again sorry I missed it for so long.

@rodrigoelp
Copy link
Contributor

Is all good

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

Successfully merging this pull request may close these issues.

None yet

3 participants