Repository navigation
fix: disable expo router sitemap in production builds #164
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
base: master
Are you sure you want to change the base?
Changes from all commits
f8aafdc
f27caf7
e42b2ac
d4045f2
0ded3f2
b0a6b07
7d1eff9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| import type { ConfigContext, ExpoConfig } from '@expo/config'; | ||
|
|
||
| type AppConfigFn = (context: ConfigContext) => ExpoConfig; | ||
|
|
||
| const loadConfigFor = (appEnv: string | undefined): ExpoConfig => { | ||
| if (appEnv === undefined) { | ||
| delete process.env.APP_ENV; | ||
| } else { | ||
| process.env.APP_ENV = appEnv; | ||
| } | ||
| let appConfig: AppConfigFn | undefined; | ||
| jest.isolateModules(() => { | ||
| appConfig = require('../app.config').default; | ||
| }); | ||
| if (!appConfig) { | ||
| throw new Error('app.config.ts could not be loaded'); | ||
| } | ||
| return appConfig({ config: {} } as ConfigContext); | ||
| }; | ||
|
|
||
| const getExpoRouterOptions = (config: ExpoConfig) => { | ||
| const entry = config.plugins?.find((plugin) => | ||
| Array.isArray(plugin) | ||
| ? plugin[0] === 'expo-router' | ||
| : plugin === 'expo-router', | ||
| ); | ||
| if (!Array.isArray(entry)) { | ||
| throw new Error( | ||
| `Expected the expo-router plugin to be configured as ['expo-router', options], received ${JSON.stringify(entry)}`, | ||
| ); | ||
| } | ||
| return entry[1]; | ||
| }; | ||
|
|
||
| describe('app.config expo-router sitemap', () => { | ||
| const originalAppEnv = process.env.APP_ENV; | ||
|
|
||
| afterEach(() => { | ||
| if (originalAppEnv === undefined) { | ||
| delete process.env.APP_ENV; | ||
| } else { | ||
| process.env.APP_ENV = originalAppEnv; | ||
| } | ||
| }); | ||
|
|
||
| it('disables the sitemap route in production', () => { | ||
| const config = loadConfigFor('production'); | ||
|
|
||
| expect(getExpoRouterOptions(config)).toEqual({ sitemap: false }); | ||
| }); | ||
|
|
||
| it.each(['development', 'qa', 'staging'])( | ||
| 'keeps the sitemap route in %s', | ||
| (appEnv) => { | ||
| const config = loadConfigFor(appEnv); | ||
|
|
||
| expect(getExpoRouterOptions(config)).toEqual({ sitemap: true }); | ||
| }, | ||
| ); | ||
|
|
||
| it('keeps the sitemap route when APP_ENV is unset (defaults to development)', () => { | ||
| const config = loadConfigFor(undefined); | ||
|
|
||
| expect(getExpoRouterOptions(config)).toEqual({ sitemap: true }); | ||
| }); | ||
|
|
||
| it('fails loudly when the expo-router entry has no options', () => { | ||
| const config = { plugins: ['expo-router'] } as ExpoConfig; | ||
|
|
||
| expect(() => getExpoRouterOptions(config)).toThrow( | ||
| /Expected the expo-router plugin/, | ||
| ); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,7 +86,7 @@ export default ({ config }: ConfigContext): ExpoConfig => ({ | |
| ], | ||
| 'expo-dev-client', | ||
| 'expo-localization', | ||
| 'expo-router', | ||
| ['expo-router', { sitemap: Env.APP_ENV !== 'production' }], | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @devs-rootstrap this gate fails open:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @fernandatoledo good catch. In 0ded3f2 I added |
||
| ['app-icon-badge', appIconBadgeConfig], | ||
| ['react-native-edge-to-edge'], | ||
| ], | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devs-rootstrap these tests only assert the plugin tuple for four explicit
APP_ENVvalues. Could you add a case for unsetAPP_ENV(which defaults todevelopment), and make theexpo-routerlookup fail loudly if the entry reverts to the bare string'expo-router'instead of returningundefined? Also worth noting that nothing here verifies the runtime behavior (/_sitemapunreachable in production), and the PR says it hasn't been checked on a device.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fernandatoledo done in b0a6b07. There is now a case for unset
APP_ENVthat expects{ sitemap: true }(env.jsfalls back todevelopment). Theexpo-routerlookup now throws if the entry is a bare string or missing instead of returningundefined, and a test covers the bare'expo-router'case. You are right that these tests only check the config and not the runtime behavior:/_sitemapbeing unreachable in a production build still needs a manual check on a device.