Skip to content

Commit

Permalink
feat: warn on duplicate screen names across navigators
Browse files Browse the repository at this point in the history
  • Loading branch information
satya164 committed Feb 21, 2021
1 parent 61c6bb0 commit 02a031e
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 17 deletions.
31 changes: 25 additions & 6 deletions packages/core/src/BaseNavigationContainer.tsx
Expand Up @@ -20,6 +20,7 @@ import useOptionsGetters from './useOptionsGetters';
import useEventEmitter from './useEventEmitter';
import useSyncState from './useSyncState';
import checkSerializable from './checkSerializable';
import checkDuplicateRouteNames from './checkDuplicateRouteNames';
import type {
NavigationContainerEventMap,
NavigationContainerRef,
Expand All @@ -32,6 +33,7 @@ const NOT_INITIALIZED_ERROR =
"The 'navigation' object hasn't been initialized yet. This might happen if you don't have a navigator mounted, or if the navigator hasn't finished mounting. See https://reactnavigation.org/docs/navigating-without-navigation-prop#handling-initialization for more details.";

const serializableWarnings: string[] = [];
const duplicateNameWarnings: string[] = [];

try {
/**
Expand Down Expand Up @@ -294,15 +296,17 @@ const BaseNavigationContainer = React.forwardRef(
});

React.useEffect(() => {
const hydratedState = getRootState();

if (process.env.NODE_ENV !== 'production') {
if (state !== undefined) {
const result = checkSerializable(state);
if (hydratedState !== undefined) {
const serializableResult = checkSerializable(hydratedState);

if (!result.serializable) {
const { location, reason } = result;
if (!serializableResult.serializable) {
const { location, reason } = serializableResult;

let path = '';
let pointer: Record<any, any> = state;
let pointer: Record<any, any> = hydratedState;
let params = false;

for (let i = 0; i < location.length; i++) {
Expand Down Expand Up @@ -344,13 +348,28 @@ const BaseNavigationContainer = React.forwardRef(
console.warn(message);
}
}

const duplicateRouteNamesResult = checkDuplicateRouteNames(
hydratedState
);

if (duplicateRouteNamesResult.length) {
const message = `Found screens with the same name in multiple navigators. Check:\n${duplicateRouteNamesResult.map(
([_, locations]) => `\n${locations.join(', ')}`
)}\n\nThis can cause confusing behavior during navigation. Consider using unique names for each screen instead.`;

if (!duplicateNameWarnings.includes(message)) {
duplicateNameWarnings.push(message);
console.warn(message);
}
}
}
}

emitter.emit({ type: 'state', data: { state } });

if (!isFirstMountRef.current && onStateChangeRef.current) {
onStateChangeRef.current(getRootState());
onStateChangeRef.current(hydratedState);
}

isFirstMountRef.current = false;
Expand Down
38 changes: 38 additions & 0 deletions packages/core/src/checkDuplicateRouteNames.tsx
@@ -0,0 +1,38 @@
import type { NavigationState, PartialState } from '@react-navigation/routers';

export default function checkDuplicateRouteNames(state: NavigationState) {
const allRouteNames = new Map<string, string[]>();

const getRouteNames = (
location: string,
state: NavigationState | PartialState<NavigationState>
) => {
state.routeNames?.forEach((name) => {
const current = allRouteNames.get(name);
const currentLocation = location ? `${location} > ${name}` : name;

if (current) {
current.push(currentLocation);
} else {
allRouteNames.set(name, [currentLocation]);
}
});

state.routes.forEach((route: typeof state.routes[0]) => {
if (route.state) {
getRouteNames(
location ? `${location} > ${route.name}` : route.name,
route.state
);
}
});
};

getRouteNames('', state);

const duplicates = Array.from(allRouteNames.entries()).filter(
([_, value]) => value.length > 1
);

return duplicates;
}
13 changes: 13 additions & 0 deletions packages/core/src/fromEntries.tsx
@@ -0,0 +1,13 @@
// Object.fromEntries is not available in older iOS versions
export default function fromEntries<K extends string, V>(
entries: (readonly [K, V])[]
) {
return entries.reduce((acc, [k, v]) => {
if (acc.hasOwnProperty(k)) {
throw new Error(`A value for key '${k}' already exists in the object.`);
}

acc[k] = v;
return acc;
}, {} as Record<K, V>);
}
12 changes: 1 addition & 11 deletions packages/core/src/getPathFromState.tsx
Expand Up @@ -4,6 +4,7 @@ import type {
PartialState,
Route,
} from '@react-navigation/routers';
import fromEntries from './fromEntries';
import type { PathConfig, PathConfigMap } from './types';

type Options = { initialRouteName?: string; screens: PathConfigMap };
Expand Down Expand Up @@ -227,17 +228,6 @@ export default function getPathFromState(
return path;
}

// Object.fromEntries is not available in older iOS versions
const fromEntries = <K extends string, V>(entries: (readonly [K, V])[]) =>
entries.reduce((acc, [k, v]) => {
if (acc.hasOwnProperty(k)) {
throw new Error(`A value for key '${k}' already exists in the object.`);
}

acc[k] = v;
return acc;
}, {} as Record<K, V>);

const getParamName = (pattern: string) =>
pattern.replace(/^:/, '').replace(/\?$/, '');

Expand Down

5 comments on commit 02a031e

@sbalay
Copy link

@sbalay sbalay commented on 02a031e Feb 25, 2021

Choose a reason for hiding this comment

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

Hey @satya164

I've following your work in the v6 roadmap and came across this change. I'd like to share an issue I see with this change.

Having repeated screen names is something that I found quite helpful when sharing screen components in different stacks:
Let's say I share ScreenA and ScreenB in different stacks (Tab1 and Tab2), in ScreenA I could do navigation.navigate('ScreenB') regardless of the stack that is being used.

However, if repeating names is considered a bad practice what would be the recommended way to handle those cases?
Something that comes to mind is adding a param that somehow represent which stack is being used and then:

// Add a isTab1 params which will let ScreenA know which stack is being used
if (route.params.isTab1) {
  // As we don't want to duplicate names, now ScreenB route is named "Tab1.ScreenB" in Tab1 stack
  navigation.navigate('Tab1.ScreenB')
} else {
  // As we don't want to duplicate names, now ScreenB route is named "Tab2.ScreenB" in Tab2 stack
  navigation.navigate('Tab2.ScreenB')
}

I've been working on type-safe navigation at Shop in Shopify, and already find it challenging to get type-safety in shared screens using v5. It would be amazing to come up with an official guide around sharing screens and I would be happy to contribute to it.

Sorry if this is not the best channel to ask these kind of questions, let me know if you think it's more suitable to create an issue in the repo or other alternatives for better communication

@satya164
Copy link
Member Author

Choose a reason for hiding this comment

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

@sbalay why not have those screens in a parent stack instead of in 2 stacks under tab1 and tab2?

@sbalay
Copy link

@sbalay sbalay commented on 02a031e Feb 25, 2021

Choose a reason for hiding this comment

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

The motivation is that I still want the tabbar to be visible when navigating to those screens.
Is there a better way of achieving it having the screens in the parent stack of the tabs navigator?

@satya164
Copy link
Member Author

Choose a reason for hiding this comment

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

that makes sense, though doesn't it mean there will be multiple instances of same screen which would lead to higher memory usage?
the original motivation for this warning was to discourage the case where people often nest a screen inside another with the same name. maybe the warning can be more relaxed to only warn for direct descendents instead of across the tree.

@sbalay
Copy link

@sbalay sbalay commented on 02a031e Feb 25, 2021

Choose a reason for hiding this comment

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

that makes sense, though doesn't it mean there will be multiple instances of same screen which would lead to higher memory usage?

That could be true in some cases, but I think there is still a use case for it. I'll expand a little bit on our specific case:

Shop has a home tab focused on order tracking, that display a list of shopping orders. When users tap on an item of that list we navigate to a screen with details of the order (Screen A).
We also have another tab focused on shopping, and among other things we show what shops the user is following. When tapping on a shop, we redirect to a screen that represents the shop and shows shopping orders made to that shop. Tapping on an order goes to order details (Screen A).

Both tabs have very different goals, so we want to have the tabbar visible to let the user go back and forth between different features.
It could happen that at some point we open the same order detail (Screen A) in both tabs, but from the app perspective it's not entirely wrong.

the original motivation for this warning was to discourage the case where people often nest a screen inside another with the same name. maybe the warning can be more relaxed to only warn for direct descendents instead of across the tree.

We also have a case for that, I can expand as well if you think these examples are helpful.

Please sign in to comment.