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] move typed-routes logic into expo-router #26578

Merged
merged 16 commits into from Feb 12, 2024

Conversation

marklawlor
Copy link
Contributor

@marklawlor marklawlor commented Jan 22, 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/clichecks 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

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Jan 22, 2024
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Jan 22, 2024
options.projectRoot,
'expo-router/build/typed-routes'
);
return typedRoutesModule ? typedRoutes(typedRoutesModule, options) : legacyTypedRoutes(options);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We switch to the expo-router version if available

@@ -383,6 +384,8 @@ export function assertDuplicateRoutes(filenames: string[]) {

/** Given a Metro context module, return an array of nested routes. */
export function getRoutes(contextModule: RequireContext, options?: Options): RouteNode | null {
options = { importMode: EXPO_ROUTER_IMPORT_MODE, ...options };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typed routes need to force async loading, so this option can now be overridden.

interface RequireContextPonyFill extends RequireContext {
__add(file: string): void;
__delete(file: string): void;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to rebuild the context every time a file changes, so I added these methods to modify the context.

"types": [
"jest",
"jest-require",
"node"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

typed-routes use node:fs and node:path, so I added node to the types. We could also remove this can just ts-ignore the imports.

@@ -0,0 +1,345 @@
/* eslint-disable */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a new centralised location for Expo Router types that can be shared by both the code-base and typed routes. After this is merged, I'd adjust the expo-router package to use these types.

@marklawlor marklawlor marked this pull request as ready for review January 22, 2024 21:43

const typedRoutesModule = require(typedRoutesModulePath);

if (metro && server) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably be an assertion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I prefer adding a comment explaining why these values could be false.

Copy link

linear bot commented Jan 29, 2024

@marklawlor marklawlor force-pushed the marklawlor/typed-routes-rewrite branch 2 times, most recently from 9dbb76b to 6d5952d Compare January 30, 2024 00:46
@marklawlor marklawlor force-pushed the marklawlor/typed-routes-rewrite branch 2 times, most recently from 136055c to 2c30c89 Compare February 12, 2024 08:11
@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions and removed bot: passed checks ExpoBot has nothing to complain about labels Feb 12, 2024
@marklawlor marklawlor force-pushed the marklawlor/typed-routes-rewrite branch from 00784a3 to b8deb57 Compare February 12, 2024 09:04
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 Feb 12, 2024
@marklawlor marklawlor merged commit 41c27d3 into main Feb 12, 2024
11 checks passed
@marklawlor marklawlor deleted the marklawlor/typed-routes-rewrite branch February 12, 2024 20:42
EvanBacon added a commit that referenced this pull request Feb 22, 2024
# Why

The logic introduced in #26578 breaks
router for all platforms in development only.

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

# How

<!--
How did you build this feature or fix this bug and why?
-->

# Test Plan

<!--
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>
@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