Skip to content
This repository has been archived by the owner on Feb 25, 2020. It is now read-only.

Added Partial to setParams #79

Closed
wants to merge 2 commits into from
Closed

Conversation

Titozzz
Copy link

@Titozzz Titozzz commented Nov 19, 2019

You might not want to update all params?

I'm open to discussing this as I don't know if this was intentional or not :)

You might not want to update them all!
You might not want to update all
@slorber
Copy link
Member

slorber commented Nov 19, 2019

Hi @Titozzz

The NavigationParams interface seems already flexible enough to me, and not sure what you expect adding Partial will provide (

To me this won't change anything, do you see any case that might differ before/after? I've made a playground here: http://www.typescriptlang.org/play/?ssl=1&ssc=1&pln=18&pc=36#code/JYOwLgpgTgZghgYwgAgHJwG7AOZzMAexAAU4o4BbAZ2QG8AoZZAbQGsIBPALmSrClDYAujzggOAbnoBfevQRE+vCGFLlqARmQBeZAAoADmUpUA-D3RZc+ImpMBKHQD46yWQpBKqKu9QBMOvpG6mY8avhwADYAPJY4eIQkxtROjtoutG5y3qrJVBp69lI5vlR+hVL0JXkFtNJFVT555XUNjbkhtVQEFBC+PBr1xU0hLd29-ciDbdWderTjfck8AK4gACYQMKAQ60PtpWM9S+qrG1s7e0VAA

@Titozzz
Copy link
Author

Titozzz commented Nov 19, 2019

But I didn't check where the generic is used so I'll have another look

@Titozzz
Copy link
Author

Titozzz commented Nov 19, 2019

The file I'd need to be updated is @react-navigation/core/lib/typescript/types.d.ts

I'd need setParams to be setParams(params: Partial<ParamList[RouteName]>): void;

export declare type NavigationProp<ParamList extends ParamListBase, RouteName extends keyof ParamList = string, State extends NavigationState = NavigationState, ScreenOptions extends object = {}, EventMap extends Record<string, any> = {}> = NavigationHelpersCommon<ParamList, State> & {
    /**
     * Update the param object for the route.
     * The new params will be shallow merged with the old one.
     *
     * @param params Params object for the current route.
     */
    setParams(params: ParamList[RouteName]): void;
    /**
     * Update the options for the route.
     * The options object will be shallow merged with default options object.
     *
     * @param options Options object for the route.
     */
    setOptions(options: Partial<ScreenOptions>): void;
    /**
     * Returns the parent navigator, if any. Reason why the function is called
     * dangerouslyGetParent is to warn developers against overusing it to eg. get parent
     * of parent and other hard-to-follow patterns.
     */
    dangerouslyGetParent<T = NavigationProp<ParamListBase> | undefined>(): T;
    /**
     * Returns the navigator's state. Reason why the function is called
     * dangerouslyGetState is to discourage developers to use internal navigation's state.
     * Note that this method doesn't re-render screen when the result changes. So don't use it in `render`.
     */
    dangerouslyGetState(): State;
} & EventConsumer<EventMap & EventMapBase> & PrivateValueStore<ParamList, RouteName, EventMap>;

@slorber
Copy link
Member

slorber commented Nov 19, 2019

setParams(params: Partial<ParamList[RouteName]>): void; seems better to me yes, as it's shallow merged we could only ask the user to pass partial params

So if I understand correctly what you do is something like:

const MyScreen = ({navigation}: {navigation: NavigationProp<{p1: string, p2: string>}>) => {

  // this work
  const handleEvent = () => navigation.setParams({p1: "x",p2: "y"});

  // this fail
  const handleEvent = () => navigation.setParams({p1: "x"});

  return ...
}

???

@satya164
Copy link
Member

Hey @Titozzz! Seems like you're using v5. This is not the correct repo. The code is here https://github.com/react-navigation/navigation-ex

@Titozzz
Copy link
Author

Titozzz commented Nov 19, 2019

🤦‍♂
EDIT: Yeah I'm gonna submit to V5
react-navigation/navigation-ex#177

@Titozzz Titozzz closed this Nov 19, 2019
@Titozzz Titozzz deleted the patch-1 branch November 19, 2019 13:43
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.

3 participants