feat(router): ENG-5727 router impl #2757

crherman7 · 2024-09-16T19:36:34Z

Describe your changes

Add implementation of routing solution as thin-client around react-native-navigation.

path based routing (bottom tab + component + action)
route guards
bottom tab + single stack layout
route context hooks
native modals
tab-affinity
global provider
pre-launch script

Issue ticket number and link

Ticket ENG-5727
Ticket ENG-5728
Ticket ENG-5729
Ticket ENG-5730

Type of change

New feature (non-breaking change which adds functionality)

Test Plan

Run the example app.

Checklist before requesting a review

A self-review of my code has been completed
Tests have been added / updated if required

packages/app-router/src/router.tsx

crherman7 · 2024-09-24T20:53:55Z

packages/app-router/src/router.tsx

+  if (!Component) return;
+
+  // Attach options to the component
+  (Component as any).options = passOptions;


deep merge with static options

packages/app-router/src/router.tsx

NickBurkhartBB · 2024-09-25T19:36:25Z

packages/app-router/src/router.tsx

+      // handle the error (e.g., log it)
+    }
+
+    Navigation.setRoot(layout);


I think we may want to separate the registration of routes and setting the layout. Having this way reduces flexibility for any app that needs some type of conditional root based on whatever data and would require patch to allow them to use this library.

I agree with this, this would be important for conditional layouts. We should likely add an additional optional parameter called autoRoot or autoLayout - which will default to true. If it is false it will then not setRoot and either way it will resolve the created layout that would be used in setRoot based on the routes supplied.

packages/app-router/src/hooks.tsx

NickBurkhartBB · 2024-09-25T20:31:05Z

packages/app-router/src/hooks.tsx

+  async function open(path: string, passProps = {}, options?: Options) {
+    const url = createAppURL(path);
+    const matchedRoute = route.routes.find(it => {
+      return match(it.path)(url.pathname);


this may be premature optimization, but should we have this path-to-regexp run once in register routes setup, not through every search loop?

packages/app-router/src/hooks.tsx

devs9 · 2024-11-03T00:21:59Z

packages/app-router/src/router.tsx

+  layout: LayoutRoot,
+  routeName: string,
+  bottomTab: any,
+  stackId?: string,


stackId is not described in JSDoc

nice catch, updated!

devs9 · 2024-11-03T00:27:00Z

packages/app-router/src/router.tsx

+
+/**
+ * Renders the component for a given route.
+ *


nit: wrong JSDoc

nice catch, updated!

devs9 · 2024-11-03T00:33:58Z

packages/app-router/src/router.tsx

+
+/**
+ * Registers a single route with the navigation system.
+ *


nit: wrong JSDoc

nice catch, updated!

devs9 · 2024-11-03T00:56:00Z

packages/app-router/src/router.tsx

+
+/**
+ * Registers routes and sets the root layout for the application.
+ *


nit: wrong JSDoc

nice catch, updated!

devs9 · 2024-11-03T12:10:40Z

packages/app-router/src/hooks.tsx

+    const name = `Modal_${idCounter}`;
+    idCounter++;


it might also be useful to optionally pass this nane as we do for route

I agree with this - but it needs more thought because we won't know for sure if the "screen" has been registered or not.

devs9 · 2024-11-03T12:26:15Z

I like this router implementation ❤️
especially the single stack layout, route guards, and providers great features 👍

packages/app-router/src/router.tsx

packages/app-router/src/types.ts

packages/app-router/src/router.tsx

Here's a brief overview of each hook: 1. `useComponentId` - This hook allows you to get the unique component ID from the current route. 2. `useLinking` - This hook provides functionality for linking between routes in your application. 3. `useModal` - This hook manages modals within your application, allowing you to open and close them as needed. 4. `useNavigator` - This hook provides a way to navigate through the application's history. 5. `usePathParams` - This hook allows you to access the dynamic path parameters from the current route. 6. `useQueryParams` - This hook enables you to access the query parameters from the current URL. 7. `useRoute` - This hook retrieves the current router state from the RouterContext. 8. `useRouteData` - This hook allows you to retrieve the router data associated with the current route.

NickBurkhartBB · 2024-11-13T14:43:27Z

packages/app-router/src/hooks/useRouteData.ts

+export function useRouteData<T>(): T {
+  const route = useRoute();
+
+  return route.data as T;


what does this add above useRoute? seems like you just get less information using this hook and could just use useRoute.

Yeah I don't like this hook either, was trying to preserve previous interface from fsapp

NickBurkhartBB · 2024-11-13T14:57:35Z

packages/app-router/src/hooks/useLinking.ts

+
+        callback({url});
+      } catch (e) {
+        // Handle the error (e.g., log it)


rather then eating this errors without any ways of modification other then patch, do you think we should add an optional handler callback function to be passed into useLinking? that way if they have custom logging around errors they could add it.

yeah probably some kind of callback would be good, good idea.

NickBurkhartBB · 2024-11-13T15:16:12Z

packages/app-router/src/hooks/useNavigator.tsx

+
+    try {
+      Navigation.registerComponent(
+        name,


I wonder if registering this component each time on show modal has a consequence of flooding the registry and causing memory issues. maybe we should keep this in mind and run a stress test in an application around this.

yeah I was thinking same thing before, I'm going to test to see about a tagging policy....that way if the component is tagged we can just render.

packages/app-router/src/hooks/usePathParams.ts

NickBurkhartBB · 2024-11-13T16:51:12Z

packages/app-router/src/hooks/useLinking.ts

+    if (!url) return;
+
+    try {
+      const {pathname, query} = urlParse(url);


I think we need to use hostname in deep links as well. I am seeing examples of apps using deep links like app-name://shop and expecting it to route to path /shop. but since shop is the host based on urlParse the path doesn't match. example in docs would route path ?user=123 instead of /profile?user=123. not sure if we want to just check protocol or have a way to customize.

so I don't think we are aligned on this, I may be miss-reading this. but this hook would only be triggered by a host that is registered natively via info.plist or manifest. can you elaborate further? maybe this conversation belongs in the open function?

After talking on the call the main issues surrounds how url-parse parses a provided URL.

https://www.google.com/search?id=abc host -> www.google.com path -> /search

myapp://app/search?id=abc host -> app path -> /search

This may cause issues depending on how the url scheme is registered and how the routes are defined. If there is no host defined in the url schemes then the path should be /app/search where is if the host is defined as app and the routes do not start with /app then the path would be /search. So we need to come up with logic that fills these parameters.

packages/app-router/src/hooks.tsx

crherman7 marked this pull request as draft September 16, 2024 19:36

NickBurkhartBB changed the base branch from develop to feature/app-router September 16, 2024 21:53

crherman7 force-pushed the feat/ENG-5727 branch from 1db85d8 to 0dda0dd Compare September 23, 2024 21:55

crherman7 commented Sep 23, 2024

View reviewed changes

packages/app-router/src/router.tsx Outdated Show resolved Hide resolved

crherman7 force-pushed the feat/ENG-5727 branch 2 times, most recently from 406700b to 1d3de35 Compare September 24, 2024 18:42

feat(router): ENG-5727 add router implementation

9bf1141

crherman7 force-pushed the feat/ENG-5727 branch from 1d3de35 to 9bf1141 Compare September 24, 2024 19:01

crherman7 changed the title ~~feat(router): ENG-5727 router register routes~~ feat(router): ENG-5727 router impl Sep 24, 2024

crherman7 commented Sep 24, 2024

View reviewed changes

fix: cleanup dependencies

fa1df7e

crherman7 marked this pull request as ready for review September 25, 2024 14:06

NickBurkhartBB reviewed Sep 25, 2024

View reviewed changes

devs9 reviewed Nov 3, 2024

View reviewed changes

crherman7 added 7 commits November 4, 2024 09:58

docs: add README.md

37e63da

fix: refactor types

2456d67

fix: update modal

b207b3e

fix: strict types

3dd8603

fix: remove unused imports

f29127b

feat: update docs

4d26722

fix: type based bottomtab

9b5932b

crherman7 commented Nov 6, 2024

View reviewed changes

packages/app-router/src/router.tsx Outdated Show resolved Hide resolved

packages/app-router/src/types.ts Outdated Show resolved Hide resolved

packages/app-router/src/router.tsx Outdated Show resolved Hide resolved

crherman7 added 3 commits November 6, 2024 10:52

fix: remove unused function

4decd83

feat: conditional root

e902645

crherman7 force-pushed the feat/ENG-5727 branch from 5df66e4 to 263da05 Compare November 12, 2024 18:35

chore(docs): update readme

d343409

crherman7 force-pushed the feat/ENG-5727 branch from 74041e5 to d343409 Compare November 12, 2024 18:55

NickBurkhartBB reviewed Nov 13, 2024

View reviewed changes

feat(navigator): modal tagging policy

5b37ce4

crherman7 force-pushed the feat/ENG-5727 branch 2 times, most recently from 64597cc to 3529712 Compare November 14, 2024 15:38

feat(linking): add optional hosts + error callback

6bff390

crherman7 force-pushed the feat/ENG-5727 branch from 3529712 to 6bff390 Compare November 14, 2024 15:51

NickBurkhartBB approved these changes Nov 18, 2024

View reviewed changes

NickBurkhartBB merged commit 64bf3c9 into brandingbrand:feature/app-router Nov 18, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(router): ENG-5727 router impl #2757

feat(router): ENG-5727 router impl #2757

crherman7 commented Sep 16, 2024 •

edited

Loading

crherman7 Sep 24, 2024

NickBurkhartBB Sep 25, 2024

crherman7 Nov 5, 2024

NickBurkhartBB Sep 25, 2024

devs9 Nov 3, 2024

crherman7 Nov 5, 2024

devs9 Nov 3, 2024

crherman7 Nov 5, 2024

devs9 Nov 3, 2024

crherman7 Nov 5, 2024

devs9 Nov 3, 2024

crherman7 Nov 5, 2024

devs9 Nov 3, 2024

crherman7 Nov 7, 2024

devs9 commented Nov 3, 2024 •

edited

Loading

NickBurkhartBB Nov 13, 2024

crherman7 Nov 13, 2024

NickBurkhartBB Nov 13, 2024

crherman7 Nov 13, 2024

NickBurkhartBB Nov 13, 2024

crherman7 Nov 13, 2024

NickBurkhartBB Nov 13, 2024

crherman7 Nov 13, 2024

crherman7 Nov 13, 2024

feat(router): ENG-5727 router impl #2757

feat(router): ENG-5727 router impl #2757

Conversation

crherman7 commented Sep 16, 2024 • edited Loading

Describe your changes

Issue ticket number and link

Type of change

Test Plan

Checklist before requesting a review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devs9 commented Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crherman7 commented Sep 16, 2024 •

edited

Loading

devs9 commented Nov 3, 2024 •

edited

Loading