Adding externalUserId to connect-react #17063
Conversation
userId should be deprecated
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThis update introduces support for a new Changes
Sequence Diagram(s)sequenceDiagram
participant ConsumerApp
participant ComponentForm
participant FormContextProvider
participant resolveUserId
participant SDK
ConsumerApp->>ComponentForm: Provide externalUserId or userId
ComponentForm->>FormContextProvider: Pass externalUserId/userId props
FormContextProvider->>resolveUserId: Resolve user ID
resolveUserId-->>FormContextProvider: {resolvedId, warningType}
FormContextProvider->>SDK: Use resolvedId as user identifier
Note over FormContextProvider: Log deprecation warning if needed
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
packages/connect-react/src/components/ControlApp.tsx (1)
30-38: 🛠️ Refactor suggestionPotential unnecessary re-fetch due to unstable input object.
useAccounts({ externalUserId, app, oauth_app_id: oauthAppId }, …)is passed a new
object every render.
IfuseAccountsforwards that object into the React-QueryqueryKey, each render
invalidates the cache and triggers a refetch.Wrap the input in
useMemo(or let the hook accept primitives) to stabilise identity:- } = useAccounts( - { - externalUserId, - app: app.name_slug, - oauth_app_id: oauthAppId, - }, + const accountInput = useMemo( + () => ({ + externalUserId, + app: app.name_slug, + oauth_app_id: oauthAppId, + }), + [externalUserId, app.name_slug, oauthAppId], + ); + } = useAccounts(accountInput,packages/connect-react/src/components/RemoteOptionsContainer.tsx (1)
60-70: 🛠️ Refactor suggestionStabilise
componentConfigureInputto prevent query-key churn.
componentConfigureInputis rebuilt on every render, which will change the
React-Query key and bypass caching even when none of the constituent values changed.Wrap in
useMemowith[externalUserId, page, context, component.key, prop.name, configuredPropsUpTo, dynamicProps?.id]
dependencies or stringify the object when building the key.packages/connect-react/src/components/ComponentForm.tsx (1)
12-35: 🛠️ Refactor suggestionRedundant optional props defeat the “exactly-one” union guarantee
externalUserIdanduserIdare first declared as optional (lines 13-20) and then constrained by the XOR-like union (lines 32-35).
Because the base object already allows both to beundefined, TypeScript will happily accept{}– breaking the “one-of-two required” intent and re-introducing the possibility of both IDs being present (theneverconstraint is weakened by the preceding?).Refactor by removing the duplicate optional fields and folding the JSDoc comments into the union itself:
-export type ComponentFormProps<T extends ConfigurableProps, U = ConfiguredProps<T>> = { - /** - * Your end user ID, for whom you're configuring the component. - */ - externalUserId?: string; - /** - * @deprecated Use `externalUserId` instead. - */ - userId?: string; - ... -} & ( - | { externalUserId: string; userId?: never } - | { userId: string; externalUserId?: never } -); +export type ComponentFormProps<T extends ConfigurableProps, U = ConfiguredProps<T>> = + { + ... + } & ( + /** + * Provide exactly one of the two IDs. + */ + | { + /** Your end-user ID (preferred) */ + externalUserId: string; + userId?: never; + } + | { + /** @deprecated ‑ use `externalUserId` instead */ + userId: string; + externalUserId?: never; + } + );This restores the compile-time guarantee that exactly one identifier is supplied.
🧹 Nitpick comments (4)
packages/connect-react/src/utils/resolve-user-id.ts (1)
9-18: Emit deprecation warning once, not on every call.Down-stream hooks may call
resolveUserIdon every render causing noisy console spam.
Consider extracting the warning logic to the form-context provider so it fires only once per form instance.packages/connect-react/src/hooks/form-context.tsx (3)
83-91: Minor: includeresolveUserIdcall inuseMemofor safety but drop superfluous arrowYou can shorten the
useMemocall:-const { resolvedId: resolvedExternalUserId, warningType } = useMemo(() => - resolveUserId(externalUserId, userId), - [externalUserId, userId] -); +const { resolvedId: resolvedExternalUserId, warningType } = useMemo( + () => resolveUserId(externalUserId, userId), + [externalUserId, userId], +);Not critical, just trims an unnecessary line break.
93-100: Console warnings will fire in every non-production bundleConsider wrapping the deprecation logs with
if (process.env.NODE_ENV !== "production")to avoid shipping noisy console output to end-users in production builds.
571-573: Back-compat field naming may mislead new consumers
userIdis still exposed (mirroringexternalUserId) to spare legacy code.
Document this clearly in code comments or plan a follow-up major version that removes the alias to prevent accidental new usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
packages/connect-react/CHANGELOG.md(1 hunks)packages/connect-react/package.json(1 hunks)packages/connect-react/src/components/ComponentForm.tsx(2 hunks)packages/connect-react/src/components/ControlApp.tsx(3 hunks)packages/connect-react/src/components/RemoteOptionsContainer.tsx(2 hunks)packages/connect-react/src/hooks/form-context.tsx(6 hunks)packages/connect-react/src/utils/resolve-user-id.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/connect-react/src/components/ControlApp.tsx (1)
packages/connect-react/src/hooks/form-context.tsx (1)
useFormContext(58-67)
packages/connect-react/src/hooks/form-context.tsx (2)
packages/connect-react/src/utils/resolve-user-id.ts (1)
resolveUserId(5-21)docs-v2/components/GlobalConnectProvider.jsx (1)
externalUserId(38-41)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
🔇 Additional comments (5)
packages/connect-react/package.json (1)
3-3: Verify ancillary version bump artefacts (lock-file, changelog tag, CI release step).The package version is bumped to
1.3.0, but it’s easy to forget:
- Publishing / release automation still points at the previous tag.
- A
pnpm-lock.yamlorpackage-lock.jsoncommit may be required.- A corresponding git tag / GitHub release should be queued.
Double-check these before merging to avoid broken downstream installs.
packages/connect-react/src/components/ControlApp.tsx (1)
32-33: Null safety forexternalUserId.
externalUserIdis typed asstring | undefined; passingundefineddown to
useAccounts(and ultimately the backend) may yield a 400.Either guard:
if (!externalUserId) throw new Error("externalUserId is required in ControlApp");or let the hook enforce it via its type signature.
packages/connect-react/src/components/RemoteOptionsContainer.tsx (1)
15-24:externalUserIdpropagation mirrors ControlApp concerns (undefined / empty).Same null-safety & warning considerations apply here. Confirm backend tolerates
undefinedor guard early.packages/connect-react/CHANGELOG.md (1)
5-18: Changelog entry looks good – ensure CI release notes pick it up.packages/connect-react/src/hooks/form-context.tsx (1)
155-160:✅ Verification successful
Double-check backend interface: was
userIdrenamed everywhere?
reloadComponentPropsInputnow sends{ externalUserId, … }. If the backend still expectsuserId, this will break dynamic-prop loading.The interface definition should list
externalUserIdinstead ofuserId. If it still listsuserId, update the SDK types or send both keys for backward compatibility.
🏁 Script executed:
#!/usr/bin/env bash # Verify the expected key of ReloadComponentPropsOpts across the repo rg -A2 -n "interface ReloadComponentPropsOpts"Length of output: 48
No definitions for
ReloadComponentPropsOptswere found. Let’s broaden the search across possible type names and usages to locate the expected key:
🏁 Script executed:
#!/usr/bin/env bash # Search for ReloadComponentProps type definitions and invocations for term in ReloadComponentProps ReloadComponentPropsInput ReloadComponentPropsOpts reloadComponentProps; do echo "### Searching for $term ###" rg -C3 -n "$term" || true doneLength of output: 15615
🏁 Script executed:
#!/usr/bin/env bash echo "### ExternalUserId definition ###" rg -n "export type ExternalUserId" -C5 packages/sdk/src/shared/index.ts || true echo echo "### reloadComponentProps payload construction ###" sed -n '1370,1420p' packages/sdk/src/shared/index.tsLength of output: 1651
Let's locate the actual
ExternalUserIddefinition to confirm which key(s) it requires:
🏁 Script executed:
#!/usr/bin/env bash echo "### Search for `type ExternalUserId` ###" rg -n "type ExternalUserId" -C5 packages/sdk/src/shared/index.ts || true echo echo "### Search for `interface ExternalUserId` ###" rg -n "interface ExternalUserId" -C5 packages/sdk/src/shared/index.ts || trueLength of output: 908
To confirm the exact shape of
ExternalUserId, let’s pull more context around its definition:
🏁 Script executed:
#!/usr/bin/env bash # Show full ExternalUserId definition (with surrounding lines) rg -n "type ExternalUserId" -C10 packages/sdk/src/shared/index.tsLength of output: 796
No action needed:
ReloadComponentPropsOptsalready handles bothexternalUserIdanduserId
- The SDK’s
ExternalUserIdtype (used byReloadComponentPropsOpts) requires at least one ofexternalUserIdoruserId.- In
reloadComponentProps, we destructureso providing onlyconst { userId, externalUserId = userId } = opts;userIdstill yields a validexternalUserId.- The HTTP payload always sends
external_user_id: externalUserId, matching the backend’s updated expectations.
* Adding externalUserId to connect-react userId should be deprecated * linting fixes * PR feedback * Update CHANGELOG.md
userId should be deprecated
WHY
Summary by CodeRabbit
New Features
Chores