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] enable testing on all platforms #27407

Merged
merged 5 commits into from Mar 13, 2024

Conversation

marklawlor
Copy link
Contributor

@marklawlor marklawlor commented Mar 4, 2024

Why

Update jest.config.js to enable tests on all platforms

Test Plan

Checklist

@marklawlor marklawlor assigned marklawlor and unassigned marklawlor Mar 4, 2024
@marklawlor marklawlor marked this pull request as ready for review March 4, 2024 01:49
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Mar 4, 2024
@@ -1,3 +1,4 @@
/** @jest-environment jsdom */
Copy link
Contributor Author

@marklawlor marklawlor Mar 4, 2024

Choose a reason for hiding this comment

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

This test uses import { render } from '@testing-library/react'; which assumes there is a DOM environment.

It looks like there is progress being made for better support for SSR and RSC

@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Mar 4, 2024
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.

Seems fine but we should avoid running agnostic tests multiple times, e.g. matchers.test.web.ts -> matchers.test.node.ts

@marklawlor marklawlor force-pushed the marklawlor/router/enable-jest-platforms branch from 76aee02 to 71df55a Compare March 12, 2024 23:03
@marklawlor marklawlor merged commit 7b34eb7 into main Mar 13, 2024
9 of 10 checks passed
@marklawlor marklawlor deleted the marklawlor/router/enable-jest-platforms branch March 13, 2024 05:27
@brentvatne brentvatne added the published Changes from the PR have been published to npm label Apr 18, 2024
marklawlor added a commit that referenced this pull request Apr 26, 2024
# Why

Revised version #26047
Blocked by #27407 

# How

Rules:

* API routes cannot have platform extensions
* Layouts and routes can have extensions only if a non-extension version
exists

For example, you have have `/page.web.tsx` only if `/page.tsx` also
exists. This makes platform extensions an additive change and won't
break concepts like universal deep linking and typed routes

API routes do not support platform extensions as they are independent of
a platform.

# Implementation

`getRoutes` allows multiple files to be assigned to the same route, but
all files were given the same specificity.

This PR adds logic to calculate the specificity of a file and to select
the most specific file.

Specificity highest-to-lowest

`2`: Platform extension matches platform
`1`: `.native` platform extension
`0`: No platform extensions
`-1`: Invalid route 

If no platform is provided to `getRoutes` the current behaviour is
retained and files with platform extensions will be ignored.

# `getRoutes` checklist

These are the locations we use `getRoutes` and whether they should use
platform extensions or not.


**[renderStaticContent](https://github.com/expo/expo/blob/269625f5b70a95656212ad14ca7f3e82b45b9f61/packages/expo-router/src/static/renderStaticContent.tsx#L19)**

- Used in `getManifest()` ❌
- Used in `getBuildTimeServerManifestAsync` ❌ - I'm not 100% sure on
this one. Need to understand how to test this correctly

**[routes-manifest](

https://github.com/expo/expo/blob/269625f5b70a95656212ad14ca7f3e82b45b9f61/packages/expo-router/src/routes-manifest.ts#L4)**

- Used in `createRoutesManifest` ❌


**[router-store](https://github.com/expo/expo/blob/269625f5b70a95656212ad14ca7f3e82b45b9f61/packages/expo-router/src/global-state/router-store.tsx#L27)**

- Used in `RouterStore` ✅


**[typed-routes](https://github.com/expo/expo/blob/269625f5b70a95656212ad14ca7f3e82b45b9f61/packages/expo-router/src/typed-routes/generate.ts#L5)**

- Used in `getTypedRoutesDeclarationFile` ❌


There is also an `getExactRoutes` function, but this is only used for
testing purposes.

** 

# 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: Evan Bacon <bacon@expo.io>
Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
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