Make Sanity Optional #80
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR aims to make the Sanity CMS integration optional for the Satus project, allowing developers to fork and build without configuring Sanity. The implementation adds conditional logic to check if Sanity is configured and provides fallback implementations when it's not.
Key Changes:
- Modified
assertValue()to log errors instead of throwing, returning undefined for missing environment variables - Added conditional client creation using
isSanityConfigured()check - Implemented fallback exports for
sanityFetch,SanityLive, and draft mode routes when Sanity is not configured
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| integrations/sanity/env.ts | Modified assertValue to return undefined and log errors instead of throwing exceptions |
| integrations/sanity/client.ts | Made client creation conditional based on isSanityConfigured(), returning null when not configured |
| integrations/sanity/live/index.tsx | Added fallback implementations for sanityFetch and SanityLive when client is null |
| integrations/sanity/fetch.ts | Added null check for client before fetching, throwing error if not configured |
| app/api/draft-mode/enable/route.ts | Made draft mode route conditional, returning 500 error response when Sanity not configured |
| app/sitemap.ts | Added null check for imported client before using it to fetch sitemap data |
Critical Issues Found: The implementation has several type safety and consistency issues that need to be addressed. The main concern is that assertValue() now returns undefined as T, which circumvents TypeScript's type system and can cause runtime errors. Additionally, there are inconsistencies in error handling patterns and missing parameter signatures in fallback functions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| browserToken: publicToken, | ||
| serverToken: privateToken, |
There was a problem hiding this comment.
When client is truthy but publicToken or privateToken are undefined (due to the modified assertValue function), defineLive will be called with undefined tokens. This could cause runtime errors or unexpected behavior in the Sanity live query functionality.
Consider adding explicit checks for required tokens:
client && publicToken && privateToken
? defineLive({...})
: {...}
app/api/draft-mode/enable/route.ts
Outdated
| export const { GET } = defineEnableDraftMode({ | ||
| client: client.withConfig({ token: privateToken }), | ||
| }) | ||
| export const { GET } = client |
There was a problem hiding this comment.
When client is truthy but privateToken is undefined (due to the modified assertValue function returning undefined), client.withConfig({ token: undefined }) will be called. This could cause issues with the draft mode functionality.
Consider checking for privateToken explicitly:
client && privateToken
? defineEnableDraftMode({
client: client.withConfig({ token: privateToken }),
})
: {...}| export const { GET } = client | |
| export const { GET } = client && privateToken |
integrations/sanity/env.ts
Outdated
| if (v === undefined) { | ||
| throw new Error(errorMessage) | ||
| console.error(errorMessage) | ||
| // throw new Error(errorMessage) |
There was a problem hiding this comment.
Commented-out code should be removed rather than left in the codebase. If you're unsure about removing the error throwing behavior, consider using a feature flag or environment variable to control it instead.
Either remove this line or implement a proper configuration option.
| if (!client) { | ||
| throw new Error('Sanity is not configured') | ||
| } |
There was a problem hiding this comment.
The error handling is inconsistent across the codebase. Here you throw an error, but in integrations/sanity/live/index.tsx:12-16 you return a resolved promise with an error object, and in app/api/draft-mode/enable/route.ts:11-15 you return a 500 response.
For consistency and better user experience, consider:
- Using the same error handling pattern across all fallback implementations
- Throwing errors early (like here) is good, but then the other fallbacks should also throw or handle errors consistently
- Document the expected behavior when Sanity is not configured
| console.error(errorMessage) | ||
| // throw new Error(errorMessage) | ||
| return undefined as T |
There was a problem hiding this comment.
The assertValue function is returning undefined as T which is a type-unsafe pattern that defeats the purpose of TypeScript's type system. This will cause runtime errors when the returned undefined values (like projectId, dataset, publicToken, studioUrl, previewURL) are used elsewhere in the code.
Instead of silencing the error and returning undefined as T, consider one of these safer approaches:
- Return a nullable type
T | undefinedand update call sites to handle null/undefined values - Return a default empty string for string values (if appropriate for your use case)
- Keep throwing the error but make the client initialization conditional on
isSanityConfigured()check first
integrations/sanity/client.ts
Outdated
| export const client = isSanityConfigured() | ||
| ? createClient({ | ||
| projectId, | ||
| dataset, | ||
| apiVersion, | ||
| useCdn: true, | ||
| perspective: 'published', | ||
| token: privateToken, | ||
| stega: { | ||
| studioUrl, | ||
| filter: (props) => { | ||
| if (props.sourcePath.at(-1) === 'title') { | ||
| return true | ||
| } | ||
|
|
||
| return props.filterDefault(props) | ||
| }, | ||
| }, | ||
| }) | ||
| return props.filterDefault(props) | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
When isSanityConfigured() returns true but environment variables are missing, createClient will be called with undefined values from the assertValue function in env.ts. This happens because assertValue now returns undefined as T instead of throwing an error.
This creates an inconsistency: isSanityConfigured() checks only for NEXT_PUBLIC_SANITY_PROJECT_ID and NEXT_PUBLIC_SANITY_DATASET, but createClient also requires publicToken, studioUrl, and previewURL which depend on other environment variables like NEXT_PUBLIC_SANITY_API_READ_TOKEN and NEXT_PUBLIC_BASE_URL.
Consider checking for all required environment variables in the conditional or handling undefined values more gracefully.
…anity client and live
This commit is meant to make Sanity totally optional, meaning that anyone forking Satus can just build the project right away.
I create a branch beause i'm not entirely sure about the implementation, mostly the
assertValuepart. Instead of throwing error and stoppig the build, this will log an error.