Skip to content

Prepare for deprecated radix-ui IdProvider#2068

Merged
zomars merged 10 commits intocalcom:mainfrom
denik1981:chore/remove-deprecated-radix-id-provider
Apr 12, 2022
Merged

Prepare for deprecated radix-ui IdProvider#2068
zomars merged 10 commits intocalcom:mainfrom
denik1981:chore/remove-deprecated-radix-id-provider

Conversation

@denik1981
Copy link
Copy Markdown
Contributor

What does this PR do?

Prepares Cal for the announced deprecated IdProvider that is was used to generate unique id between SSR and client hydration and helps prevent the React mismatch error.

Cal codebase not only is using IdProvider for radix components but also the useId hook provided by the same radix package.
However, the useId hook can still be used to generate unique Ids for (radix and non-radix) components as it is used currently in fields.tsx and Switch.tsx, and it is not dependent on IdProvider as its context. Check the source code.

As a follow-up, once React 18 becomes stable and used by Cal, this PR can be revisited to replace the radix version of useId with the new native React.useId hook.

Type of change

  • New feature (non-breaking change which adds functionality)

How should this be tested?

  • yarn test
  • yarn test e2e
  • Also checked the rendered html and lookup for unique id in where it is used.

Checklist

  • I haven't added tests that prove my fix is effective or that my feature works

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 5, 2022

@denik1981 is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 5, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

docs – ./apps/docs

🔍 Inspect: https://vercel.com/cal/docs/HqA359XkCmfSrRB571dRgbvzCRwd
✅ Preview: https://docs-git-fork-denik1981-chore-remove-deprecated-radi-6fcd9c-cal.vercel.app

[Deployment for a6dfc90 canceled]

calendso – ./apps/web

🔍 Inspect: https://vercel.com/cal/calendso/TbzWRMa9jxy7CTh4aomD2vQwjPhy
✅ Preview: https://calendso-git-fork-denik1981-chore-remove-deprecated-f02fc7-cal.vercel.app

@vercel vercel Bot temporarily deployed to Preview – docs March 5, 2022 22:51 Inactive
@denik1981
Copy link
Copy Markdown
Contributor Author

I'm wondering if I should add code comments as a refactoring remainder to replace useId[radix] with useId[react18] once we start using react18.

@PeerRich
Copy link
Copy Markdown
Member

just to confirm: all you did was remove ID?

@vercel vercel Bot temporarily deployed to Preview – docs March 14, 2022 11:53 Inactive
@denik1981
Copy link
Copy Markdown
Contributor Author

just to confirm: all you did was remove ID?

Yup. I've only removed the wrapper. It is doing nothing.

I have previously checked with the Radix team as well cause Cal is using a radix internal hook to generate unique ids which we still need to use it to prevent SSR id mismatch errors. This hook is not marked as deprecated and is not dependent on the IdProvider. In fact, IdProvider, currently (I linked the radix code in this PR) is only logging the deprecated warning to the console. No business logic at all.

If needed, we could also replaced their id hook with a custom one ( I don't think is necessary) or wait for the upcoming useId hook in React18 (when stable and available).

@PeerRich
Copy link
Copy Markdown
Member

Yup. I've only removed the wrapper. It is doing nothing.

Ok, is it safe to merge @denik1981? or do we need to wait for something else first? or update any package

@denik1981
Copy link
Copy Markdown
Contributor Author

Safe to merge. Nothing else is needed from our side. Leaving or removing it won't make any difference besides cleaning up our console from their warning message. However, this PR will be necessary if Radix decided to remove IdProvider in a future release.

From radix source.
IdProvider is only rendering the children and throwing a warning to the console.

// DEPRECATED
function IdProvider({ children }: { children: React.ReactNode }) {
  React.useEffect(() => {
    if (process.env.NODE_ENV === 'development') {
      console.warn(ID_PROVIDER_DEPRECATED);
    }
  }, []);
  return <>{children}</>;
}

@vercel vercel Bot temporarily deployed to Preview – calendso March 15, 2022 15:26 Inactive
@vercel vercel Bot temporarily deployed to Preview – docs March 17, 2022 21:19 Inactive
@vercel vercel Bot temporarily deployed to Preview – calendso April 12, 2022 16:49 Inactive
@zomars zomars enabled auto-merge (squash) April 12, 2022 16:49
@vercel vercel Bot temporarily deployed to Preview – docs April 12, 2022 17:14 Inactive
@vercel vercel Bot temporarily deployed to Preview – calendso April 12, 2022 17:14 Inactive
@vercel vercel Bot temporarily deployed to Preview – docs April 12, 2022 17:15 Inactive
@vercel vercel Bot temporarily deployed to Preview – calendso April 12, 2022 18:34 Inactive
@zomars zomars merged commit e1df207 into calcom:main Apr 12, 2022
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.

4 participants