Skip to content

Commit

Permalink
fix: prevent double renders when pushing stacks. (#24147)
Browse files Browse the repository at this point in the history
# Why

Prevent double renders by cloning state to avoid leaking state between
functions.

- fix expo/router#838 
- fix expo/router#733
- fix expo/router#320
- The issue expo/router#320 has multiple
different things linked, but the original case appears to be fixed.
- possibly also addresses expo/router#847

<!--
Please describe the motivation for this PR, and link to relevant GitHub
issues, forums posts, or feature requests.
-->

# How

- Prevent mutating the input state to avoid invalidating the nested
state.

# Test Plan

- The testing library doesn't seem to support this case. @marklawlor has
been tasked with ensuring the original branch can detect the error
https://github.com/expo/expo/compare/%40evanbacon/router/fix-838
Just in case the testing library isn't fixed, I ran locally with:

- `app/_layout.js`, `app/(a)/_layout.js`, `app/b/_layout.js`
```js
import { Slot } from "expo-router";
export default function RootLayout() {
  return <Slot />
}
```
- `app/(a)/index.js`
```js
import { router, useNavigation } from "expo-router";
import {  View } from "react-native";

export default function App() {
  // const navigation = useNavigation();

  setTimeout(() => {
    router.push("/b");
    // navigation.push("b");
  });
  return (
    <View />
  );
}
```
- `app/b/index.js`
```js
import { usePathname } from 'expo-router';
import { Text, View } from 'react-native';

let i = 0;
export default function Page() {
  const path = usePathname();
  i++;
  return (
    <View style={{ flex: 1, backgroundColor: i > 1 ? "red" : "white" }}>
      <Text>Path: {path}</Text>
    </View>
  );
}
```



<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
  • Loading branch information
EvanBacon and expo-bot authored Aug 28, 2023
1 parent ddebf8c commit 2a0be76
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 3 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

### 🐛 Bug fixes

- Prevent double renders when pushing stacks. ([#24147](https://github.com/expo/expo/pull/24147) by [@EvanBacon](https://github.com/EvanBacon))
- Patch `react-native-web` AppContainer to prevent adding extra divs. ([#24093](https://github.com/expo/expo/pull/24093) by [@EvanBacon](https://github.com/EvanBacon))
- Allow pushing "sibling" routes by the same name. ([#23833](https://github.com/expo/expo/pull/23833) by [@EvanBacon](https://github.com/EvanBacon))
- Prevent throwing in `canGoBack` before the navigation has mounted. ([#23959](https://github.com/expo/expo/pull/23959) by [@EvanBacon](https://github.com/EvanBacon))
Expand Down
5 changes: 4 additions & 1 deletion build/fork/getPathFromState.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion build/fork/getPathFromState.js.map

Large diffs are not rendered by default.

52 changes: 52 additions & 0 deletions src/fork/__tests__/getPathFromState-upstream.test.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,58 @@ type State = PartialState<NavigationState>;
});
});

it(`does not mutate incomplete state during invocation`, () => {
const inputState = {
stale: false,
type: 'stack',
key: 'stack-xxx',
index: 1,
routeNames: ['(tabs)', 'address'],
routes: [
{
name: '(tabs)',
state: {
stale: false,
type: 'stack',
key: 'stack-zzz',
index: 0,
routeNames: ['index'],
routes: [{ name: 'index', path: '', key: 'index-xxx' }],
},
key: '(tabs)-xxx',
},
{
key: 'address-xxx',
name: 'address',
params: { initial: true, screen: 'index', path: '/address' },
},
],
};

const staticCopy = JSON.parse(JSON.stringify(inputState));
getPathFromState(
// @ts-expect-error
inputState,
{
screens: {
'(tabs)': { path: '(tabs)', screens: { index: '' } },
address: {
path: 'address',
screens: { index: '', other: 'other' },
initialRouteName: 'index',
},
_sitemap: '_sitemap',
'[...404]': '*404',
},
preserveDynamicRoutes: false,
preserveGroups: false,
}
);

expect(inputState).toEqual(staticCopy);
expect(JSON.stringify(inputState)).not.toMatch(/UNKNOWN/);
});

it(`supports resolving nonexistent, nested synthetic states into paths that cannot be resolved`, () => {
expect(
getPathFromState(
Expand Down
6 changes: 5 additions & 1 deletion src/fork/getPathFromState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,11 @@ function getPathFromResolvedState(
while (current) {
path += '/';

const route = current.routes[current.index ?? 0] as CustomRoute;
// Make mutable copies to ensure we don't leak state outside of the function.
const route = {
...(current.routes[current.index ?? 0] as CustomRoute),
};

// NOTE(EvanBacon): Fill in current route using state that was passed as params.
// if (isInvalidParams(route.params)) {
if (!route.state && isInvalidParams(route.params)) {
Expand Down

0 comments on commit 2a0be76

Please sign in to comment.