Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[router]: Refactor getRoutes #26507

Merged
merged 20 commits into from
Feb 5, 2024
Merged

[router]: Refactor getRoutes #26507

merged 20 commits into from
Feb 5, 2024

Conversation

marklawlor
Copy link
Contributor

@marklawlor marklawlor commented Jan 19, 2024

Why

With the introduction of Platform Routes, the getRoutes function needs to be updated with additional rules/checks. This is a refactor before we start on that body of work.

Current bugs

Closes ENG-8005

Current DX issues

The existing error messages are very vague. I've taking this as an opportunity to improve them

Duplicate Route errors

The difference is now it tells you which two files are in conflict and why they are in conflict. It also leaves out redundant information like route priority.

Previous

Higher priority Layout Route "./(a,c)/_layout" overriding redundant Layout Route "./(a,b)/_layout". Remove the Layout Route "./(a,b)/_layout" to fix this.

New

The layouts "./(a,c)/_layout" and "./(a,b)/_layout" conflict on the route "./(a,c)/_layout". Please remove or rename one of these files.

Deprecated _layout format

This format is now allowed.

Previous editThis error is confusing as it can occur when your not trying to use `_layout`

I'm not sure if I like this. It means we're kinda heading into everything is page.tsx territory.

app/
├─ [a].js
├─ [a]/[b].js

Previous

Using deprecated Layout Route format: Move `./app/[a].js` to `./app/[a]/_layout.js`

New

The route file `./app/[a].js` should be renamed to `./app/[a]/index.js`
```</details>


### Unknown `initialRouteName`

// new error message
"Layout ./_layout.js has invalid initialRouteName 'c'. Valid options are: 'a', 'b'"

// new error message if using group syntax
"Layout ./(a,b)/_layout.js has invalid initialRouteName 'd' for group '(b)'. Valid options are: 'c'"


# How

`getRoutes` was previously a multi-pass algorithm that had three main phases.

1. Expand the `require.context.keys()` into a file system tree
2. Traverse the file system tree "hoisting" routes to the nearest `_layout`
3. Calculating initialRoutes and entry points

The new `getRoutes` removes the extra passes and performs all logic with the three phases mentioned above. It utilises `Set`/`Map` for faster look-ups.

## Specificity

The only new concept is "specificity". When we build the file system tree in the initial phase, a single "filepath" is now an array of `RouteNode` sorted by specificity (lowest to highest). **Currently specificity is hard-coded to 0**. When we add Platform Routes, these will have a high specificity and allow us to select the correct route and assert for fallback routes.

**Previous**

type TreeNode = {
name: string;
children: TreeNode[];
parents: string[];
/** null when there is no file in a folder. */
node: FileNode | null;
};


**New**

type DirectoryNode = {
layout?: RouteNode[];
subdirectories: Map<string, DirectoryNode>;
files: Map<string, RouteNode[]>; // Each file is an array of RouteNode sorted by specificity
};


# Test Plan

The test suite for `getRoutes` has been completely written.The existing tests were my biggest issue with `getRoutes` as they were very unclear **what** they were testing. This means certain behaviours of `getRoutes()` are unclear.

Instead of testing each phase individually, the tests for `getRoutes()` have been rewritten to work more like integration tests, where we provide a context and assert against the returned schema. This makes it much easier to visualise what is happening.

# 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).

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Jan 19, 2024
@marklawlor marklawlor self-assigned this Jan 19, 2024
marklawlor and others added 9 commits January 29, 2024 14:01
Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Jan 29, 2024
@marklawlor marklawlor marked this pull request as ready for review January 29, 2024 07:58
@marklawlor marklawlor marked this pull request as draft January 29, 2024 08:13
Copy link

linear bot commented Jan 29, 2024

@marklawlor marklawlor marked this pull request as ready for review January 29, 2024 23:00
children: [],
contextKey: './(app)/(index)/blog/abc.tsx',
contextKey: './(app)/(index,about)/blog/abc.tsx',
Copy link
Contributor

Choose a reason for hiding this comment

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

contextKey is used as a unique identifier, it looks like it's possible for duplicates now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

contextKey is being used both as an ID and as a filename for various purposes.

The only place its used as an ID is the useContextKey() hook for the navigators. I don't think this breaks their functionality, but we so don't have great test coverage for this 🤔

I'll add an id attribute to RouteNode which was the previous contextKey value and update useContextKey to return the ID value.

Copy link
Contributor

@EvanBacon EvanBacon left a comment

Choose a reason for hiding this comment

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

Generally seems fine, depending on tests for a number of the advanced cases.

@marklawlor marklawlor merged commit f8c99b7 into main Feb 5, 2024
9 checks passed
@marklawlor marklawlor deleted the marklawlor/router/v4-get-routes branch February 5, 2024 22:31
marklawlor added a commit that referenced this pull request Feb 12, 2024
# Why

Recreation of #25654. I'm splitting that PR into two PRs.

1. Move typed-routes logic
2. Refactor Expo Router so the code-base and typed routes shared the
same definition file.

I've also removed the custom directory-traversal logic and switched to
using `getRoutes()` directly. This is in preparation of
 #26507 which fixes issue with the type generation.

Close ENG-10759

# How

**Previous**

- Typed routes used its own directory walk logic to generate the paths
for typed routes.
- All logic lives in `@expo/cli`

**new**

- `@expo/cli`checks for the existence of
`expo-router/build/typed-routes`. If so, it uses it instead of the
current logic.
- The updated functions call `getRoutes()` and traverse the `RouteNode`
instead of the file-system.

# Test Plan

This PR only tests the integration between `getRoutes()` and the
`expo-router.d.ts`. The next PR will add type tests that are in #25654

# 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>
@brentvatne brentvatne added the published Changes from the PR have been published to npm label Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: fingerprint compatible bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants