SSR compatibility patch for useId(...)

[benjroy]

I'm evaluating @react-aria for use in NerdWallet's frontend stack.

First, I'd like to complement you all. This project is incredible. The semantics of the hooks have me hooked. The documentation for this project is incredibly valuable: https://react-spectrum.adobe.com/react-aria

We need SSR support in order to use this package. When I came across the following comment, it made me want to test the waters of how responsive this project is to external contributions. If y'all are receptive, we'd love to contribute more patches for SSR compatibility.

Hey thanks for reporting this! SSR support is an area we’re actively working on. We’ve just started a test suite to ensure we catch all of these issues that result from running in a non-DOM environment. See #835. If you’d like to contribute tests/fixes for these issues it would be greatly appreciated! 😃

#842 (comment)

---

This PR patches the useId(id) hook to be SSR compatible. It removes the use of Math.random() to seed the ids it generates in order to keep the generated id consistent between server-response and client hydration. It replaces the random number with the current package version of @react-aria/utils, in order to keep the spirit from the comment in that file of having something visible for devs to see that multiple instances of the module are in use.

I hooked up a local babel plugin to inject the package version during babel transpilation

This patch solves ~60% of the ssr tests that are currently failing.

yarn test:ssr before:

Test Suites: 26 failed, 9 passed, 35 total
Tests:       29 failed, 9 passed, 38 total
Snapshots:   0 total
Time:        135.882s

yarn test:ssr after:

Test Suites: 12 failed, 23 passed, 35 total
Tests:       12 failed, 26 passed, 38 total
Snapshots:   0 total
Time:        74.253s, estimated 129s

P.S. for anyone else reading, the discussion concerning isomorphic ids in this react issue helped with context of this kind of issue. facebook/react#5867

✅ Pull Request Checklist:

• Included link to corresponding React Spectrum GitHub Issue.
• Added/updated unit tests and storybook for this change (for new code or code which already has tests).
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
• Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

yarn test:ssr

Related Issues:

#760
#835
#899
#842

[snowystinger]

when we publish with lerna, this'll get updated, so no need to do it in PR

[benjroy]

perfect. I couldn't tell it this project auto-versioned or not.

[snowystinger]

Did you try this? https://github.com/sindresorhus/read-pkg-up

[benjroy]

No I haven't, but that looks exactly like what I wanted. I'll add a commit to use that instead.

Does this project have a preference of it's-fine-to-include-a-dependency vs. this-is-simple-no-need-for-another-dependency?

[snowystinger]

so this will avoid collisions with different versions of the same module, but doesn't do anything for the case where two copies of the same version are loaded, which is something we might see in a micro frontend architecture
we'll need to still do something to protect against that
Unfortunately, that seems to indicate the need for a global :(
If you have any ideas, feel free to post them in thread here, I'll also confer with my team and see if they have more ideas

[benjroy]

so this will avoid collisions with different versions of the same module, but doesn't do anything for the case where two copies of the same version are loaded, which is something we might see in a micro frontend architecture

exactly. I ran into this just the other day when I was de-duping a dependency tree in one of our apps. This scenario can definitely happen, but it can also be solved (in npm with some fun dependency updating and/or npm dedupe usage, or in yarn with atlassian's yarn-deduplicate tool), but that is just a workaround for this issue that will certainly arise.

Unfortunately, that seems to indicate the need for a global :(

I agree. A WeakMap could be used to hold onto the versions for the different instances in play.

The question that remains is what should the expected behavior be? It could:

• throw an error when a duplicate instance of same version is in use (force devs to dedupe)
• print a warning when a duplicate instance of same version is in use (nicer to dev workflow)
• allow multiple instances of the same version. both instances share the same counter
• allow multiple instances of the same version, duplicate instances append -{n} to the id before the counter.

A lot of this seems like overkill. the simplest seems to be:

• allow multiple instances of the same version. both instances share the same counter

and at that point, probably don't even need to add the version number in there. just a global counter for the id variable that all module instances can share from. (this comment has me thinking this could all be that much simpler case: facebook/react#5867 (comment) .)

[snowystinger]

Thanks for looking into this and sharing how much is fixed with just this little change!

[devongovett]

Nice! I see two potential issues here, in addition to the multiple copies issue discussed above:

1. The id counter probably needs to be reset for each request on the server, otherwise it will continue to be incremented across multiple requests and won't match the counter on the client.
2. The order of calls to useId could be different, e.g. in the case of async loaded component trees. This could mean the ids wouldn't match.

One possibility is a context to store the value to increment. Applications would be required to wrap their app in a provider component, which would set the value to zero, and be incremented by each call to useId. This would solve the multiple copies issue and the id reset issue. To solve the ordering issue with async components, you'd also need to wrap each async boundary in a new context as well. This could have a different prefix so that the ids inside each async component remain consistent as well.

All that sounds pretty complicated though, and would expose a lot of implementation details to application authors, increasing the potential for bugs. An alternative we might be able to get away with would be to just return null from useId on the server, and set the id in a useEffect on the client after mounting. That would mean server rendered components wouldn't be completely accessible prior to hydration, but this would require a lot less work on the application authors part. Since our components already have so much client side functionality, they wouldn't be terribly useful without JS anyway, so this seems ok to me.

In the future, we should be able to use React's useOpaqueIdentifier hook which is designed to support this exact use case. It doesn't appear to be supported in the current version of react we have though, so will need to wait until our minimum requirement supports it.

What do you think of these options?

[benjroy]

An alternative we might be able to get away with would be to just return null from useId on the server, and set the id in a useEffect on the client after mounting. That would mean server rendered components wouldn't be completely accessible prior to hydration, but this would require a lot less work on the application authors part. Since our components already have so much client side functionality, they wouldn't be terribly useful without JS anyway, so this seems ok to me.

This sounds like the best option to me, and that would allow the Math.random() usage to remain untouched, and also negate the need for this babel plugin

[benjroy]

Added the change to useEffect instead of useMemo, and even more ssr tests are reported passing now 👍:

Test Suites: 8 failed, 27 passed, 35 total
Tests:       8 failed, 30 passed, 38 total
Snapshots:   0 total
Time:        37.39s, estimated 76s
Ran all test suites with tests matching "^((?!v2).)*$".

the the multiple copies issue discussed above still exists

[benjroy]

removed the local babel plugin from this implementation, so with useEffect instead of useMemo and keeping the Math.random() seed for the multiple instances protection, yarn test:ssr reports:

Test Suites: 16 failed, 19 passed, 35 total
Tests:       19 failed, 19 passed, 38 total
Snapshots:   0 total
Time:        55.756s
Ran all test suites with tests matching "^((?!v2).)*$".

the babel plugin was getting in the way of the build:icons step that runs at during yarn postinstalls. Hopefully this build makes it into the circle ci tests with this latest change.

[benjroy]

Lots of failing tests in the circle ci run. This module runs deep.

[snowystinger]

I have a feeling you're going to run into a problem pretty quickly right here https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/utils/src/useId.ts#L61
at least as one of the things causing failures

[matamatanot]

When I use useMeter with Next.js, I got an Error.
Warning: Prop id did not match. Server: "react-aria-2182254204-10" Client: "react-aria-1828557377-2"
Will this PR solve the problem?

💻 Code Sample

import { useMeter } from "@react-aria/meter";

type Props = {
  label: string;
  value: number;
  minValue: number;
  maxValue: number;
};

export const Meter = (props: Props) => {
  const { label, value, minValue, maxValue } = props;
  const { meterProps, labelProps } = useMeter({
    value,
    label,
    minValue,
    maxValue,
  });

  const percentage = (value - minValue) / (maxValue - minValue);
  const barWidth = `${Math.round(percentage * 100)}%`;

  return (
    <div {...meterProps} style={{ width: 200 }}>
      <div style={{ display: "flex", justifyContent: "space-between" }}>
        {label && <span {...labelProps}>{label}</span>}
        {!!label && <span>{meterProps["aria-valuetext"]}</span>}
      </div>
      <div style={{ height: 10, background: "gray" }}>
        <div style={{ width: barWidth, height: 10, background: "green" }} />
      </div>
    </div>
  );
};

import { Meter } from "../utils/Meter";

const Index = () => {
  return (
    <div>
      <Meter label={"test"} value={40} minValue={0} maxValue={100} />
    </div>
  );
};

export default Index;

[devongovett]

@benjroy so, I did a bit of investigation here, and basically concluded that the returning null approach wasn't going to work, or at least not easily. As evidenced by the failing tests, too many things depend on useId always returning a string, and not waiting for an extra useEffect before its available.

I've gone ahead and implemented the other suggestion above in #992. Basically you'll need to wrap your application in an <SSRProvider>, which will ensure consistent ids. If an <SSRProvider> is not there, then id creation will work exactly how it does today. This will allow entirely client-rendered apps to support multiple instances via a random prefix, while server rendered apps will ensure consistent ids with a counter that resets on each request. Hopefully that approach works for you.

Feel free to provide your feedback on the PR! Thanks again for getting this started. 😄