Skip to content
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

fix: conflicting nested path params in state for linking #11849

Conversation

rexfordessilfie
Copy link
Contributor

Motivation
When a path configuration contains multiple path parameters with the same name, the last path parameter value wins when constructing the navigation state.

For example consider the following scenario:

const path = '/foo/42/bar/43';

  const config = {
    initialRouteName: 'Foo',
    screens: {
      Foo: {
        path: 'foo/:id',
        screens: {
          Bar: {
            path: 'bar/:id',
          },
        },
      },
    },
  };

  const state = {
    routes: [
      {
        name: 'Foo',
        params: { id: '43' }, // PROBLEM: This should be 42
        state: {
          routes: [
            {
              name: 'Bar',
              params: { id: '43' },
              path,
            },
          ],
        },
      },
    ],
  }

This PR updates the resolution logic to assign positions to each of the path segments, so that we can assign the correct values to the screen in the navigation state based on the position which it appears in the path, and not just the name.

Test plan

  • Copy the test in this PR, checkout to main and run the tests
  • You will notice that the test fails to resolve the right params to the route 'Foo'
  • Now checkout to this PR's branch and run all tests and notice that they pass

Considerations

  • Another approach could be to include validation logic that prevents this scenario from happening by validating that there are no repeated path params with the same name.
    • This however, could be a breaking change, and might call for additional functionality as "aliasing" to be able to name path params in the config, and then translate them to a different name before being passed to the screen.
    • Alternatively, in the implementing screen, the developer could update the route param name to match what is in the new path parameter
    • OR, in the implementing screen, accept either the original param name or deep link param name and pick whichever one is available

Happy to adjust this PR to the validation approach if desired, or close in favor of the user-land mitigations. Thanks!

@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.03%. Comparing base (b146aed) to head (2d8f29f).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11849      +/-   ##
==========================================
+ Coverage   76.99%   77.03%   +0.04%     
==========================================
  Files         207      207              
  Lines        6151     6162      +11     
  Branches     2420     2425       +5     
==========================================
+ Hits         4736     4747      +11     
  Misses       1361     1361              
  Partials       54       54              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

netlify bot commented Feb 25, 2024

Deploy Preview for react-navigation-example ready!

Name Link
🔨 Latest commit 10a32b8
🔍 Latest deploy log https://app.netlify.com/sites/react-navigation-example/deploys/65eb14d4a606f70008463695
😎 Deploy Preview https://deploy-preview-11849--react-navigation-example.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR

packages/core/src/getStateFromPath.tsx Outdated Show resolved Hide resolved
packages/core/src/getStateFromPath.tsx Outdated Show resolved Hide resolved
packages/core/src/getStateFromPath.tsx Outdated Show resolved Hide resolved
packages/core/src/getStateFromPath.tsx Outdated Show resolved Hide resolved
@rexfordessilfie rexfordessilfie force-pushed the fix-conflicting-nested-path-params branch from 07979af to 5a87f8e Compare March 7, 2024 10:09
@rexfordessilfie rexfordessilfie changed the title fix: resolve conflicting nested path params in state for linking fix: conflicting nested path params in state for linking Mar 8, 2024
Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@satya164 satya164 enabled auto-merge (squash) March 8, 2024 13:38
@satya164 satya164 merged commit aeaadd6 into react-navigation:main Mar 8, 2024
8 checks passed
satya164 added a commit that referenced this pull request Mar 9, 2024
**Motivation**
When a path configuration contains multiple path parameters with the
same name, the last path parameter value wins when constructing the
navigation state.

For example consider the following scenario:

```ts
const path = '/foo/42/bar/43';

  const config = {
    initialRouteName: 'Foo',
    screens: {
      Foo: {
        path: 'foo/:id',
        screens: {
          Bar: {
            path: 'bar/:id',
          },
        },
      },
    },
  };

  const state = {
    routes: [
      {
        name: 'Foo',
        params: { id: '43' }, // PROBLEM: This should be 42
        state: {
          routes: [
            {
              name: 'Bar',
              params: { id: '43' },
              path,
            },
          ],
        },
      },
    ],
  }
```

This PR updates the resolution logic to assign positions to each of the
path segments, so that we can assign the correct values to the screen in
the navigation state based on the position which it appears in the path,
and not just the name.

**Test plan**
* Copy the test in this PR, checkout to main and run the tests
* You will notice that the test fails to resolve the right params to the
route `'Foo'`
* Now checkout to this PR's branch and run all tests and notice that
they pass

**Considerations**
* Another approach could be to include validation logic that prevents
this scenario from happening by validating that there are no repeated
path params with the same name.
* This however, could be a breaking change, and might call for
additional functionality as "aliasing" to be able to name path params in
the config, and then translate them to a different name before being
passed to the screen.
* Alternatively, in the implementing screen, the developer could update
the route param name to match what is in the new path parameter
* OR, in the implementing screen, accept either the original param name
or deep link param name and pick whichever one is available

Happy to adjust this PR to the validation approach if desired, or close
in favor of the user-land mitigations. Thanks!

---------

Co-authored-by: Satyajit Sahoo <satyajit.happy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants