From 4d0c989d86074762eaf33e934ea37b7ed1002edf Mon Sep 17 00:00:00 2001
From: Stephanie Freitag <stephanie.freitag@holi.team>
Date: Tue, 18 Feb 2025 14:59:56 +0100
Subject: [PATCH] NOISSUE: make sync event propagation explicit and add tests
---
.../components/HoliLink/index.web.tsx | 6 +-
.../components/__tests__/HoliLink.test.tsx | 10 --
.../__tests__/HoliTextLink.test.tsx | 10 --
.../hooks/__tests__/useLink.test.ts | 104 ++++++++++++++++++
core/navigation/hooks/useLink.ts | 18 +--
5 files changed, 119 insertions(+), 29 deletions(-)
create mode 100644 core/navigation/hooks/__tests__/useLink.test.ts
diff --git a/core/navigation/components/HoliLink/index.web.tsx b/core/navigation/components/HoliLink/index.web.tsx
index 1559c1aa2a..517cee01e7 100644
--- a/core/navigation/components/HoliLink/index.web.tsx
+++ b/core/navigation/components/HoliLink/index.web.tsx
@@ -15,12 +15,14 @@ export const HoliLink = ({
}: PropsWithChildren<HoliLinkProps>) => {
const onLinkPress = useCallback(
async (event: SyntheticEvent<HTMLAnchorElement, Event>) => {
- const stopPropagation = await onPress?.(event)
- if (stopPropagation === false) {
+ // Async onPress callbacks won't be awaited before event propagation
+ const propagateEventOrPromise = onPress?.(event)
+ if (propagateEventOrPromise === false) {
event?.preventDefault()
event?.stopPropagation()
return false
}
+ await propagateEventOrPromise
},
[onPress]
)
diff --git a/core/navigation/components/__tests__/HoliLink.test.tsx b/core/navigation/components/__tests__/HoliLink.test.tsx
index bd042e8c0f..4dd504a907 100644
--- a/core/navigation/components/__tests__/HoliLink.test.tsx
+++ b/core/navigation/components/__tests__/HoliLink.test.tsx
@@ -31,14 +31,4 @@ describe('HoliLink', () => {
expect(mockNavigate).toHaveBeenCalledWith('//spaces')
})
-
- it('should navigate on press if event is not propagated', () => {
- const onPress = jest.fn().mockReturnValue(false)
- render(<HoliLink label="test" href="/" onPress={onPress} />)
-
- fireEvent.press(screen.getByTestId('link'))
-
- expect(onPress).toHaveBeenCalled()
- expect(mockNavigate).not.toHaveBeenCalled()
- })
})
diff --git a/core/navigation/components/__tests__/HoliTextLink.test.tsx b/core/navigation/components/__tests__/HoliTextLink.test.tsx
index dbd82b7b43..f3df698301 100644
--- a/core/navigation/components/__tests__/HoliTextLink.test.tsx
+++ b/core/navigation/components/__tests__/HoliTextLink.test.tsx
@@ -30,14 +30,4 @@ describe('HoliTextLink', () => {
expect(mockNavigate).toHaveBeenCalledWith('//spaces')
})
-
- it('should navigate on press if event is not propagated', () => {
- const onPress = jest.fn().mockReturnValue(false)
- render(<HoliTextLink label="test" href="/" onPress={onPress} size="md" />)
-
- fireEvent.press(screen.getByRole('link'))
-
- expect(onPress).toHaveBeenCalled()
- expect(mockNavigate).not.toHaveBeenCalled()
- })
})
diff --git a/core/navigation/hooks/__tests__/useLink.test.ts b/core/navigation/hooks/__tests__/useLink.test.ts
new file mode 100644
index 0000000000..73ac82e950
--- /dev/null
+++ b/core/navigation/hooks/__tests__/useLink.test.ts
@@ -0,0 +1,104 @@
+import useLink from '@holi/core/navigation/hooks/useLink'
+import useRouting from '@holi/core/navigation/hooks/useRouting'
+import { renderHook, waitFor } from '@testing-library/react-native'
+import { Linking, Platform } from 'react-native'
+
+const mockUseRouting = jest.mocked(useRouting)
+jest.mock('@holi/core/navigation/hooks/useRouting', () => jest.fn())
+
+describe('useLink', () => {
+ const testEvent = {} as React.SyntheticEvent
+ const mockNavigate = jest.fn()
+ const mockNavigateBack = jest.fn()
+
+ beforeEach(() => {
+ const routingMocks: ReturnType<typeof useRouting> = {
+ navigate: mockNavigate,
+ navigateBack: mockNavigateBack,
+ replaceRoute: jest.fn(),
+ replaceHistory: jest.fn(),
+ setParams: jest.fn(),
+ }
+ mockUseRouting.mockReturnValue(routingMocks)
+ })
+
+ it('should call onPress if given', () => {
+ const onPress = jest.fn()
+ const { result } = renderHook(() => useLink({ href: '/', onPress }))
+
+ result.current(testEvent)
+
+ expect(onPress).toHaveBeenCalled()
+ })
+
+ it('should stop event propagation', () => {
+ const onPress = jest.fn().mockReturnValue(false)
+ const { result } = renderHook(() => useLink({ href: '/', onPress }))
+
+ result.current(testEvent)
+
+ expect(onPress).toHaveBeenCalled()
+ expect(mockNavigate).not.toHaveBeenCalled()
+ })
+
+ it('should not await async onPress callback before deciding on event propagation', async () => {
+ const onPress = jest.fn().mockResolvedValue(false)
+ const { result } = renderHook(() => useLink({ href: '/', onPress }))
+
+ result.current(testEvent)
+
+ expect(onPress).toHaveBeenCalled()
+ await waitFor(() => expect(mockNavigate).toHaveBeenCalled())
+ })
+
+ it('should propagate event if not stopped', async () => {
+ const onPress = jest.fn()
+ const { result } = renderHook(() => useLink({ href: '/', onPress }))
+
+ result.current(testEvent)
+
+ expect(onPress).toHaveBeenCalled()
+ await waitFor(() => expect(mockNavigate).toHaveBeenCalledWith('//'))
+ })
+
+ it('should navigate to href', () => {
+ const { result } = renderHook(() => useLink({ href: '/' }))
+
+ result.current(testEvent)
+
+ expect(mockNavigate).toHaveBeenCalledWith('//')
+ })
+
+ it('should open external url', () => {
+ const mockOpenURL = jest.spyOn(Linking, 'openURL')
+ const { result } = renderHook(() => useLink({ href: '/', external: true }))
+
+ result.current(testEvent)
+
+ expect(mockNavigate).not.toHaveBeenCalled()
+ expect(mockOpenURL).toHaveBeenCalledWith('/')
+ })
+
+ it('should navigate back first on mobile', async () => {
+ const { result } = renderHook(() => useLink({ href: '/spaces', mobileNavigateBackFirst: true }))
+
+ result.current(testEvent)
+
+ expect(mockNavigateBack).toHaveBeenCalled()
+ await waitFor(() => expect(mockNavigate).toHaveBeenCalledWith('//spaces'))
+ })
+
+ it('should ignore domain and locale path param on mobile', () => {
+ const originalPlatform = Platform.OS
+ Platform.OS = 'android'
+
+ const { result } = renderHook(() => useLink({ href: 'https://app.holi.social/en/spaces' }))
+
+ result.current(testEvent)
+
+ expect(mockNavigate).toHaveBeenCalledWith('/spaces')
+
+ // Clean up
+ Platform.OS = originalPlatform
+ })
+})
diff --git a/core/navigation/hooks/useLink.ts b/core/navigation/hooks/useLink.ts
index 2a964935eb..c5ccbc14f0 100644
--- a/core/navigation/hooks/useLink.ts
+++ b/core/navigation/hooks/useLink.ts
@@ -6,7 +6,7 @@ import useRouting from '@holi/core/navigation/hooks/useRouting'
export interface UseLinkProps {
href: string
- /** if given, `onPress` will be executed before opening href (if given). if `onPress` returns `false` event propagation will be stopped */
+ /** if given, `onPress` will be executed before opening href (if given). if `onPress` returns `false` (synchronously, required for web) event propagation will be stopped */
onPress?: (event: React.SyntheticEvent | GestureResponderEvent) => unknown | Promise<unknown>
external?: boolean
mobileNavigateBackFirst?: boolean
@@ -18,8 +18,10 @@ const useLink = ({ onPress, external, href, mobileNavigateBackFirst }: UseLinkPr
return useCallback(
async (event: React.SyntheticEvent | GestureResponderEvent) => {
if (onPress) {
- const stopPropagation = await onPress(event)
- if (stopPropagation === false) return
+ // Not awaiting promise before event propagation to mimick web behaviour
+ const propagateEventOrPromise = onPress(event)
+ if (propagateEventOrPromise === false) return
+ await propagateEventOrPromise
}
if (external) {
Linking.openURL(href)
@@ -29,10 +31,12 @@ const useLink = ({ onPress, external, href, mobileNavigateBackFirst }: UseLinkPr
if (Platform.OS === 'ios' || Platform.OS === 'android') {
// If the platform is mobile and the link domain matches our app's domain, navigate to the route instead of opening the link
const route = '/' + href.replace(/https:\/\/(app\.holi\.social|staging\.dev\.holi\.social)(\/(en|de))?\/?/, '')
- navigate(route)
- } else if (mobileNavigateBackFirst) {
- navigateBack()
- setTimeout(() => navigate(href), 300)
+ if (mobileNavigateBackFirst) {
+ navigateBack()
+ setTimeout(() => navigate(route), 300)
+ } else {
+ navigate(route)
+ }
} else {
navigate(href)
}
--
GitLab