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

Add TypeScript declaration file. #5276

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
@janhesters
Copy link

janhesters commented Nov 23, 2018

Motivation

As discussed in this issue we should manage the TypeScript typings from within react-navigation.

This file extends the old DefinitelyTyped file:

What I added (and needs to be checked / reviewed): unmountInactiveRoutes, drawerType and missing contentOptions for DrawerItems.
Added defaultNavigationOptions.
Added default params inside route definitions.
Added headerBackgroundTransitionPreset.
Added cardOverlayEnabled and cardShadowEnabled.
Added createAppContainer (thanks to @BrendonSled).
Added Context and types in package.json (thanks to @bySabi).

What I couldn't add:
The support for React Hooks (I didn't find the code in V3).
Type definitions for createAppContainer (I didn't have time to look into it and understand the code).

TODO:

  1. Changelog
  2. React Hooks
  3. See this comment below for how you can help.

Changelog

TODO: Add an entry under the "Unreleased" heading in CHANGELOG.md which explains your change.

Add TypeScript declaration file.
As discussed in [this issue](react-navigation/react-navigation-core#2) we should manage the TypeScript typings from within react-navigation.

This file extends the old DefinitelyTyped file:

What I added (and needs to be checked / reviewed): `unmountInactiveRoutes`, `drawerType` and missing `contentOptions` for `DrawerItems`.
Added `defaultNavigationOptions`.
Added default params inside route definitions.
Added 'headerBackgroundTransitionPreset'.
Added `cardOverlayEnabled` and `cardShadowEnabled`.

What I couldn't add:
The support for React Hooks (I didn't find the code in V3).
Type definitions for `createAppContainer` (I didn't have time to look into it and understand the code).

@janhesters janhesters referenced this pull request Nov 23, 2018

Open

Move to TypeScript #2

@react-navigation-ci

This comment has been minimized.

@bySabi

This comment has been minimized.

Copy link

bySabi commented Nov 26, 2018

@janhesters React Hooks support are in a separated package https://github.com/react-navigation/react-navigation-hooks.
Maybe rewrite it with typescript will be better than create a .d.ts file.
I will try to contrib on this but my typescript is very limited at present time.

@jhalborg

This comment has been minimized.

Copy link

jhalborg commented Nov 26, 2018

@bySabi - Rewriting to TS is a much larger task, even if it is the way to move forward. A proper declaration file that is maintained along with this repo instead of separately by the community on DefinitlyTyped is a great start, as it removes the "lag" there is when new updates come out

@bySabi

This comment has been minimized.

Copy link

bySabi commented Nov 26, 2018

My fault, sorry! .. I refer to React Hook package only, https://github.com/react-navigation/react-navigation-hooks, not the whole react-navigation

@react-navigation-ci

This comment has been minimized.

Show resolved Hide resolved src/index.d.ts
add createAppContainer
Co-Authored-By: janhesters <31096420+janhesters@users.noreply.github.com>
@react-navigation-ci

This comment has been minimized.

@Sharlaan

This comment has been minimized.

Copy link

Sharlaan commented Nov 28, 2018

Really missing this createAppContainer typing... TS prevents compilation because of this missing type.

@janhesters

This comment has been minimized.

Copy link

janhesters commented Nov 28, 2018

@Sharlaan It's been added today in this PR 😊

@Sharlaan

This comment has been minimized.

Copy link

Sharlaan commented Nov 28, 2018

i saw but is not merged ::)

@BrendonSled
Copy link

BrendonSled left a comment

A prop was missing

Show resolved Hide resolved src/index.d.ts

bySabi and others added some commits Nov 30, 2018

Update src/index.d.ts
Co-Authored-By: janhesters <31096420+janhesters@users.noreply.github.com>
@janhesters

This comment has been minimized.

Copy link

janhesters commented Nov 30, 2018

@BrendonSled Thank you!

@react-navigation-ci

This comment has been minimized.

@janhesters

This comment has been minimized.

Copy link

janhesters commented Dec 2, 2018

@brentvatne Will this be merged? Or is there anything missing? 😊

@react-navigation-ci

This comment has been minimized.

@brentvatne

This comment has been minimized.

Copy link
Member

brentvatne commented Dec 3, 2018

@janhesters - I would like to merge this but what makes me a little bit hesitant is that it will go out of date easily without us having some automated checks in place. For example, we used to have a precommit hook that ran flow on the NavigationPlayground project to do a quick sanity check. Ideally we would change the project to use TypeScript and run tsc on it in precommit / in CI. If you can do this in this PR or if you are willing to commit to opening up a follow-up PR for it soon, then I can land this.

@janhesters

This comment has been minimized.

Copy link

janhesters commented Dec 3, 2018

@brentvatne Sorry english is not my first language, so just asking for clarification:

Can I rewrite React Navigation completely to TypeScript? No, as this would take way too much time. That should probably be a group effort (I am willing to help 😊).

If you mean can I rewrite the NavigationPlayground to use TypeScript, yes of course I can try. Kindly point me to it and I will get to work! 😊

@brentvatne

This comment has been minimized.

Copy link
Member

brentvatne commented Dec 3, 2018

If you mean can I rewrite the NavigationPlayground to use TypeScript, yes of course I can try. Kindly point me to it and I will get to work! 😊

yup, this one! :) https://github.com/react-navigation/react-navigation/tree/master/examples/NavigationPlayground

@react-navigation-ci

This comment has been minimized.

@janhesters

This comment has been minimized.

Copy link

janhesters commented Dec 6, 2018

@brentvatne Took some time but I did ~80% of the migration work. The Playground is now in TypeScript. For personal reasons I can't put more time into this, but this should be a major start. Someone just has to review / finish it.

Here are the steps that still have to be done:

  1. Some people who are experts on TS (I'm only using it since 4 months) should look over the declarations file and the PlayGround and review it.
  2. I don't know how to use Expo and have never used is. The new Playground is without Expo. If it needs Expo, some expert on Expo has to add that.
  3. The following types have just been lazily anyed by me, because I didn't have time to properly type them:
export interface HeaderStyleInterPolator {
  forLayout: any;
  forLeft: any;
  forLeftButton: any;
  forLeftLabel: any;
  forCenterFromLeft: any;
  forCenter: any;
  forRight: any;
  forBackground: any;
  forBackgroundWithInactiveHidden: any;
  forBackgroundWithFade: any;
  forBackgroundWithTranslation: any;
}

export const getActiveChildNavigationOptions: any;

They have to be changed in the declaration file.
4. There are four routes missing from the PlayGround right now that I couldn't migrate CustomTranisitioner, TabsWithNavigationFocus, TabsWithNavigationEvents and KeyboardHandlingExample. They still have to be migrated and rewritten with TS.

Hope this helps!

Most of the index.d.ts files works. Maybe after some more review, we should just merge it and let the community add the missing times over time (if there are any) and fix the type mistakes.

@jonbretman

This comment has been minimized.

Copy link

jonbretman commented Dec 6, 2018

@janhesters @brentvatne I can have a go at picking this up - getting the typescript definitions published as part of this package would be a huge win for us so worth the time.

@kcolton

This comment has been minimized.

Copy link

kcolton commented Dec 6, 2018

OMG THANK YOU

Thank you all who have maintained the existing DefinitelyTyped typings and for prioritizing types in general! 🙇 🙇 This is not an easy package to type, but they have been soooo helpful in development and the type declaration file nearly a pinned tab in IDE

some thoughts:

maintenance:

@brentvatne have some experience w/ maintaining type declaration files and watching the cycles of drift and correction for a bunch of packages. As long as there is a decent community of people using typescript who are active then it usually winds up working out pretty well. You can always fannegal the types a bit if a little off too.

A PR into this repo (as well as tracking if work is already in progress) is going to be more approachable than into DefinitelyTyped.

Perhaps a pull request template with a checkboxes like:

  • Updated type definitions? (if applicable)
  • Updated examples/playground? (if applicable)

as reminders for contributors to update types and examples if they are changing interfaces.

Getting full coverage for react-navigation seems like it could be an NP-complete problem :P But the best practices from DefinitelyTyped are a great start and should be easily runnable for people and CI alike.

And since in the same repo, the same release tag will freeze typings as they are. A general rule of thumb for "if new feature requires changing typing, is at least a minor version bump" would allow for fixes to types for current release to go into a patch.

Type files that are a little flexible around the edges also make things easier overall. Existing type definitions seem to fit that well.

examples/playground:

idk much about the examples and playground setup yet. For someone new to this repo and setup, having the examples included with core typings change makes the diff hard to digest (low signal to noise IMHO) and intimidating (130+ files! I thought I misread and there was a TS rewrite or something :P). Maybe because it's also first-time setup code?

Def not saying to 🗑 the examples; think they are very valuable. Thoughts on moving into a separate branch/pr or something to separate the core change and supporting materials? Just a thought from the peanut gallery - feel free to ignore

@janhesters Pretty deeply familiar with TS and react-navigation - I'll help build off the great "start" (looks closer to a finish than a start :P)

@brentvatne

This comment has been minimized.

Copy link
Member

brentvatne commented Dec 14, 2018

hi all! @kcolton - one concern i have with updating the issue template like that is that we probably need to do it on all react-navigation repos - eg: react-navigation-stack. the react-navigation package itself just brings together a bunch of repos these days. i'm not sure what a good workflow is for this, open to ideas. happy to merge this pr if we fix the conflicting files, looking forward to a typescript future, we can sort other issues in follow up, i will just need some help to get a good process in place for making sure this is all up to date and getting navigationplayground fully converted to ts

@janhesters

This comment has been minimized.

Copy link

janhesters commented Dec 14, 2018

Looks like the CI check fails because the new playground app is written without Expo. As I said, I never used it, so someone has to convert it if we still need to use it.

I could also separate this PR from the playground rewrite in case you want to merge the index.d.ts separately.

Other than that (as mentioned above) people with more skill in TS need to review this PR and add the missing screens for the playground app.

PS: In any case in about two weeks I'm gonna test out the index.d.ts file privately for a new up and see if I can find any errors. But this will probably just result in minor tweaks.

@alamothe

This comment has been minimized.

Copy link

alamothe commented Dec 18, 2018

For those of us who are super anxious (like me), here is how you can make use of @janhesters awesome typings today.

  1. Get raw index.d.ts file from the commit. Link to download is here.
  2. Make a new file inside your project named react-navigation.d.ts.
  3. Put this inside it:
declare module 'react-navigation' {
  // Copy/paste downloaded file here
  //
  // You will also need to fix two errors (remove "declare" from lines 99 and 100
}
  1. Profit! 🥇
@janhesters

This comment has been minimized.

Copy link

janhesters commented Dec 18, 2018

@alamothe Let me know if while using it you find any type errors so we can fix them (, or if you can suggest changes to this PR with the fixes).

@@ -984,6 +984,9 @@ export function TabNavigator(
routeConfigMap: NavigationRouteConfigMap,
drawConfig?: TabNavigatorConfig
): NavigationContainer;
export function createAppContainer(
routeConfigMap: NavigationRouteConfigMap,

This comment has been minimized.

@alamothe

alamothe Dec 19, 2018

Shouldn't it take a single component (of type NavigationContainer?). As far as I understand, createAppContainer cannot take a route config map.

@alamothe

This comment has been minimized.

Copy link

alamothe commented Dec 19, 2018

DefinitelyTyped had a couple of commits, and the version got bumped to 3.0.0. Please merge latest changes (it has properly typed createAppContainer).

@janhesters

This comment has been minimized.

Copy link

janhesters commented Dec 19, 2018

@alamothe Done

@react-navigation-ci

This comment has been minimized.

@timsawtell-sportsbet

This comment has been minimized.

Copy link

timsawtell-sportsbet commented Jan 10, 2019

Anything I can do to help this one along? Eagerly waiting this PR before we upgrade to v3 :)

drawerPosition?: 'left' | 'right';
contentComponent?: React.ComponentType<DrawerItemsProps>;
contentOptions?: any;
style?: StyleProp<ViewStyle>;

This comment has been minimized.

@koenpunt

koenpunt Jan 10, 2019

Contributor

drawerType and overlayColor are missing here, and drawerWidth also accepts a function;

drawerType?: 'front' | 'back' | 'slide';
overlayColor?: string;
drawerWidth?: number | (() => number);
@lucsky

This comment has been minimized.

Copy link

lucsky commented Jan 18, 2019

Is it me or are wrapped scrollables (see https://reactnavigation.org/docs/en/scrollables.html) missing from the types definition ?

@bySabi

This comment has been minimized.

Copy link

bySabi commented Jan 18, 2019

@lucsky your are right is not any type on Scrollables on current Typescript or Flow definitions.

Scrollables come from react-navigation-native https://github.com/react-navigation/react-navigation-native/blob/master/src/Scrollables.js . I'm porting this module to TS and all the types there will be exported.
I have ported react-navigation@core to TS too but is no yet merged or reviewed. Any insight will be welcome: https://github.com/bySabi/react-navigation-core/tree/move_to_typescript

@bySabi

This comment has been minimized.

Copy link

bySabi commented Jan 18, 2019

@alamothe @timsawtell-sportsbet @koenpunt I hope that https://github.com/bySabi/react-navigation-core/tree/move_to_typescript branch will be the future base of typings for react-navigation

Any help reviewing the code will be great.

@brentvatne

This comment has been minimized.

Copy link
Member

brentvatne commented Jan 18, 2019

i'm going to have to convert the navigationplayground app back to using expo as i keep it in expo in order to be able to publish it so users can open it from the expo client in ios simulator and on android device/simulator, if someone wants to do this it'd help speed up merging this

@jimrejnor

This comment has been minimized.

Copy link

jimrejnor commented Jan 20, 2019

I have installed latest version of @types/react-navigation (3.0.1), but I can't seem to find getNavigation method. I'm working on MobX navigation state management so it would be nice to have @types for that method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment