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

fix(sdk-react): make useSignal work with SSR #660

Merged
merged 2 commits into from
Feb 14, 2025

Conversation

optimistiks
Copy link
Contributor

Current behavior

A component that uses useSignal will error out during server-side rendering. For example, a simple Client Component like this in NextJS App Router (though the problem is not specific to NextJS)

// SimpleComponent.jsx

'use client'

function SimpleComponent() {
  const isMounted = useSignal(backButton.isMounted)
  return <div>Back button is {isMounted ? 'mounted' : 'not mounted'}</div> 
}

// page.jsx

function Page() {
  ...
  return (
    <div>
      <SimpleComponent />
    </div>
  )
}

This will produce the following error

Error: Missing getServerSnapshot, which is required for server-rendered content. Will revert to client rendering.

Desired behavior

The real value is only available in a browser, but the component should be able to render with some initial value on the server.

Solution

from useSyncExternalStore docs

optional getServerSnapshot: A function that returns the initial snapshot of the data in the store. It will be used only during server rendering and during hydration of server-rendered content on the client.
If you omit this argument, rendering the component on the server will throw an error.

useSyncExternalStore has getServerSnapshot argument exactly for this case. The proposed solution adds an optional second argument to useSignal that can be used if someone needs SSR. If SSR is not needed, the argument can be omitted.

Example usage when SSR is needed

const isMounted = useSignal(backButton.isMounted, false)

Example usage when SSR is not needed

const isMounted = useSignal(backButton.isMounted)

Breaking changes

None

Copy link

changeset-bot bot commented Feb 12, 2025

🦋 Changeset detected

Latest commit: b364831

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@telegram-apps/sdk-react Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Feb 12, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 14, 2025 5:20pm

@heyqbnk
Copy link
Member

heyqbnk commented Feb 14, 2025

It seems like we can just pass the signal as the third argument. We surely can't use the implementation you are providing as long as it may break in case, T is literally undefined.

So, the possible solution may be something like this:

import { useSyncExternalStore } from 'react';

/**
 * Returns the underlying signal value updating it each time the signal value changes.
 * @param signal - a signal.
 * @param getServerSnapshot - function returning the signal value snapshot. This value is only used
 * by React on the server side. Defaults to signal itself.
 */
export function useSignal<T>(
  signal: {
    (): T;
    sub(fn: VoidFunction): VoidFunction;
  },
  getServerSnapshot?: () => T,
): T {
  return useSyncExternalStore(
    (onStoreChange) => signal.sub(onStoreChange),
    signal,
    getServerSnapshot || signal,
  );
}

In case, it looks fine to you, you can update the PR and I will merge it

…ssing an optional getServerSnapshot function or the signal itself to useSyncExternalStore
@optimistiks
Copy link
Contributor Author

Thank you for your review @heyqbnk , I've updated the PR with the suggested changes.

I would like to add a test case of some sort, but it seems like first the project needs either an integration of @testing-library/react with the sdk-react package (which seems like an overkill for a package consisting of just one little hook), or an addition of the SSR case to the Next.js playground, but it seems the playground needs to be updated for the v3 version of the SDK first. Let me know what you think is the best way.

@heyqbnk heyqbnk merged commit 626f803 into Telegram-Mini-Apps:master Feb 14, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants