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 #25654

Closed
wants to merge 1 commit into from

Conversation

marklawlor
Copy link
Contributor

@marklawlor marklawlor commented Nov 30, 2023

Why

Expo Router's typed routes are not very maintainable. They are a separate set of types that need to be constantly manually kept in sync. This PR is a proposal to move the logic of typed routes into expo-router and simply have the @expo/cli clone a file.

How

First lets explain how typed routes work. TypeScript's types are like a stack. The value of a type is the first value that typescript resolves. You can override a modules types by providing your own declare module <package> definitions. However those types will have dependencies, and those values will be resolved relative to itself. So you cannot override the dependencies of types, you need to override the exported values. This is why we were keeping types manually sync'd.

This proposal moves all the types into expo-router and have the @expo/cli simply copy the type definitions and modify some lines. This way the types generated by @expo/cli are exactly the same as Expo Router.

To make this process easy, we need to centralise all expo-router types used by typed routes into a single file. I've created an ambient namespace ExpoRouter that is globally accessible in the expo-router package. Its quite easy to transform a namespace to a module as they share the same syntax.

try {
let types = await fs.readFile(path.resolve('expo-router/typed-routes.d.ts'), 'utf-8');
types = types
// Swap from being a namespace to a module
.replace('declare namespace ExpoRouter {', `declare module "expo-router" {`)
// Add the route values
.replace(
'type StaticRoutes = string;',
`type StaticRoutes = ${setToUnionType(staticRoutes)};`
)
.replace(
'type DynamicRoutes<T extends string> = string;',
`type DynamicRoutes<T extends string> = ${setToUnionType(dynamicRoutes)};`
)
.replace(
'type DynamicRouteTemplate = string;',
`type DynamicRouteTemplate = ${setToUnionType(dynamicRouteTemplates)};`
);
await fs.writeFile(path.resolve(typesDir, './router.d.ts'), types);
} catch {}
},

Pros

  • Types are kept in sync
  • Types are versioned by expo-router and no longer rely on your @expo/cli version
  • expo-router uses the more complex typed-routes declarations, making dog fooding easier

Cons:

  • Slightly more verbose syntax
  • Types are declared separately from the functions
  • expo-router uses the more complex typed-routes declarations, making development slightly harder
  • ambient types can be finicky

Feedback wanted

  • How do we maintain current versions?
    • Should this be v3 only and we simply manual fix the types for v2?
    • The ClI could check if its expo-router v2 (use old logic) or v3 (use new logic)

We just check of expo-router/types/expo-router.d.ts. If its present we use it

Copy link

linear bot commented Nov 30, 2023

ENG-10759 Ensure generated type exports match expo-router standard exports

These should never be out of sync, currently there are a number of types that appear when typed routes is enabled.

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Nov 30, 2023
params: Record<string, any>
): Omit<Required<HrefObject>, 'query'> {
params: ExpoRouter.InputRouteParams<string>
): Required<ExpoRouter.HrefObject<{ pathname: string }>> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example of where types are slightly more complex

@marklawlor marklawlor changed the title refactor(router): move typed-routes logic into expo-router [router] move typed-routes logic into expo-router Nov 30, 2023
Comment on lines 3 to 9
declare namespace ExpoRouter {
import type { ReactNode } from 'react';
import type { TextProps } from 'react-native';

type StaticRoutes = string;
type DynamicRoutes<T extends string> = string;
type DynamicRouteTemplate = never;
Copy link
Member

Choose a reason for hiding this comment

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

Note: These are non-blocking notes, code LGTM 👍
Just leaving this here in case we come back to this

I chatted with @marklawlor about this in regards to:

  • this being a d.ts (which we currently need in our approach)
  • the potential of isolating this namespace to a module explicitly
  • applying interface declaration merging to the routes here and separating this from the generated declaration file.

To keep it short, I believe we've got some wiggle room here in how this could be tweaked and implemented differently.
This should give us some options if we run into any situation where we don't like TypeScript errors or have some bad errors when module/declaration issues are introduced in user code.

All of the mentioned changes have some drawbacks and declaration merging would also imply we'd split this declaration here from a declaration that contains the generated routes. So, they're not trivial changes in how this is approached.
But, if users ever see any overly complex TypeScript errors that aren't clear enough, we can try some of the changes above (or others) to tweak it.

};
}

module.exports = withWatchPlugins({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@EvanBacon I'm getting this error when I'm trying to use withWatchPlugins and the tsd test runner

 FAIL  src/__typetests__/typed-routes.test.ts
  ● Test suite failed to run

    prompt() {
              return `select which PLATFORMS to run ${chalk.italic(this._getActiveProjectsText())}`;
            } could not be cloned.

      at ChildProcessWorker.send (../../node_modules/jest-worker/build/workers/ChildProcessWorker.js:402:17)

Tests pass when I run as a single project. Just fail when I run all projects. Is there another config option I need on the jest project?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think watch plugins need to be applied to the individual projects or the entire project, but not both.

@marklawlor marklawlor marked this pull request as ready for review December 4, 2023 08:46
byCedric added a commit that referenced this pull request Dec 19, 2023
Edit: Chatted with @marklawlor, there is a [PR that moves this script
into `expo-router`](#25654). We
probably want to merge that instead to solve this.

Edit2: We still need to backport this to SDK 50, reopening this for now.

# Why

`@expo/cli` does not have a direct dependency reference to `lodash`, yet
we import it in
[`src/start/server/type-generation/routes.ts`](https://github.com/expo/expo/blob/23842ab218b08f556b9931152e78c045d31d399e/packages/%40expo/cli/src/start/server/type-generation/routes.ts#L2).
This breaks isolated modules.

The current implicit dependency chains are:
- `@expo/cli → @expo/devcert → lodash`
- `@expo/cli → @expo/prebuild-config → expo-module-scripts → jest-expo →
lodash`
- Edit, `expo-module-scripts` here is added as peer dependency, which
seems like an error - See PR #25994
- `@expo/cli → json-schema-deref-sync → lodash`

# How

- Added `lodash.debounce` as dependency to `@expo/cli`

# Test Plan

See if `yarn typecheck` has any typescript issues.

# 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).
byCedric added a commit that referenced this pull request Dec 19, 2023
Edit: Chatted with @marklawlor, there is a [PR that moves this script
into `expo-router`](#25654). We
probably want to merge that instead to solve this.

Edit2: We still need to backport this to SDK 50, reopening this for now.

# Why

`@expo/cli` does not have a direct dependency reference to `lodash`, yet
we import it in
[`src/start/server/type-generation/routes.ts`](https://github.com/expo/expo/blob/23842ab218b08f556b9931152e78c045d31d399e/packages/%40expo/cli/src/start/server/type-generation/routes.ts#L2).
This breaks isolated modules.

The current implicit dependency chains are:
- `@expo/cli → @expo/devcert → lodash`
- `@expo/cli → @expo/prebuild-config → expo-module-scripts → jest-expo →
lodash`
- Edit, `expo-module-scripts` here is added as peer dependency, which
seems like an error - See PR #25994
- `@expo/cli → json-schema-deref-sync → lodash`

# How

- Added `lodash.debounce` as dependency to `@expo/cli`

# Test Plan

See if `yarn typecheck` has any typescript issues.

# 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).
onizam95 pushed a commit to onizam95/expo-av-drm that referenced this pull request Jan 15, 2024
Edit: Chatted with @marklawlor, there is a [PR that moves this script
into `expo-router`](expo#25654). We
probably want to merge that instead to solve this.

Edit2: We still need to backport this to SDK 50, reopening this for now.

# Why

`@expo/cli` does not have a direct dependency reference to `lodash`, yet
we import it in
[`src/start/server/type-generation/routes.ts`](https://github.com/expo/expo/blob/23842ab218b08f556b9931152e78c045d31d399e/packages/%40expo/cli/src/start/server/type-generation/routes.ts#L2).
This breaks isolated modules.

The current implicit dependency chains are:
- `@expo/cli → @expo/devcert → lodash`
- `@expo/cli → @expo/prebuild-config → expo-module-scripts → jest-expo →
lodash`
- Edit, `expo-module-scripts` here is added as peer dependency, which
seems like an error - See PR expo#25994
- `@expo/cli → json-schema-deref-sync → lodash`

# How

- Added `lodash.debounce` as dependency to `@expo/cli`

# Test Plan

See if `yarn typecheck` has any typescript issues.

# 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).
@marklawlor marklawlor force-pushed the marklawlor/ENG-10759 branch from d289e7a to d3be23e Compare January 22, 2024 00:41
@expo-bot
Copy link
Collaborator

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: Missing changelog entries


Your changes should be noted in the changelog. Read Updating Changelogs guide and consider adding an appropriate entry to the following changelogs:


Generated by ExpoBot 🤖 against d3be23e

@marklawlor marklawlor closed this Jan 22, 2024
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants