Skip to content

Commit

Permalink
fix: linking on initial path plus wildcard (#11844)
Browse files Browse the repository at this point in the history
**Motivation**

Given a linking config with same screen name at different levels and a
wildcard at one level, the derived navigation state does not always
resolve to the correct screen. This PR makes a change to include a check
for the pattern in such cases where there are two different path configs
with the same screen name.

Its a very niche edge case which I stumbled into trying to implement
another feature and I thought I would share this fix in case others are
running into the issue.


**Test plan**

To verify that existing functionality is failing, first checkout to
`main`, then copy the added test, paste it in
`packages/core/src/__tests__/getStateFromPath.test.tsx`, execute the
tests and note that it fails.

Now, checkout to this branch run all tests and note that the added test
passes.


**Considerations**
* If the logic for sorting normalized configs
[here](https://github.com/react-navigation/react-navigation/blob/fe4d4289e2bdec7614c6f2e5ca95d9b8f714af39/packages/core/src/getStateFromPath.tsx#L138)
prioritized the config map with a wildcard in the test case scenario,
this bug should not occur. However, modifying that sorting logic to make
an exception for this scenario feels unsafe (to me)!

---------

Co-authored-by: Satyajit Sahoo <satyajit.happy@gmail.com>
  • Loading branch information
rexfordessilfie and satya164 committed Mar 6, 2024
1 parent b07f0e2 commit 12624a2
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 3 deletions.
35 changes: 35 additions & 0 deletions packages/core/src/__tests__/getStateFromPath.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2345,6 +2345,41 @@ it('uses nearest parent wildcard match for unmatched paths', () => {
).toEqual(changePath(state, '/404'));
});

it('matches screen with overlapping initial path and wildcard', () => {
const path = '/bar/42/baz/test/whatever';
const config = {
screens: {
Foo: {
screens: {
Bar: {
path: '/bar/:id/',
screens: {
Baz: 'baz',
},
},
Baz: '/bar/:id/*',
},
},
},
};

const state = {
routes: [
{
name: 'Foo',
state: {
routes: [{ name: 'Baz', params: { id: '42' }, path }],
},
},
],
};

expect(getStateFromPath<object>(path, config)).toEqual(state);
expect(
getStateFromPath<object>(getPathFromState<object>(state, config), config)
).toEqual(changePath(state, '/bar/42/Baz'));
});

it('throws if two screens map to the same pattern', () => {
const path = '/bar/42/baz/test';

Expand Down
12 changes: 9 additions & 3 deletions packages/core/src/getStateFromPath.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -287,16 +287,22 @@ const matchAgainstConfigs = (remaining: string, configs: RouteConfig[]) => {
);

routes = config.routeNames.map((name) => {
const config = configs.find((c) => c.screen === name);
const params = config?.path
const routeConfig = configs.find((c) => {
// Check matching name AND pattern in case same screen is used at different levels in config
return c.screen === name && config.pattern.startsWith(c.pattern);
});

const params = routeConfig?.path
?.split('/')
.filter((p) => p.startsWith(':'))
.reduce<Record<string, any>>((acc, p) => {
const value = matchedParams[p];

if (value) {
const key = p.replace(/^:/, '').replace(/\?$/, '');
acc[key] = config.parse?.[key] ? config.parse[key](value) : value;
acc[key] = routeConfig.parse?.[key]
? routeConfig.parse[key](value)
: value;
}

return acc;
Expand Down

0 comments on commit 12624a2

Please sign in to comment.