Add ClientProvider, useClient, and useClientCapability#1607
Open
mcintyre94 wants to merge 1 commit intomainfrom
Open
Add ClientProvider, useClient, and useClientCapability#1607mcintyre94 wants to merge 1 commit intomainfrom
ClientProvider, useClient, and useClientCapability#1607mcintyre94 wants to merge 1 commit intomainfrom
Conversation
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
🦋 Changeset detectedLatest commit: f473f42 The changes in this PR will be included in the next version bump. This PR includes changesets to release 47 packages
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 |
BundleMonFiles updated (10)
Unchanged files (137)
Total files change +2.51KB +0.48% Final result: ✅ View report in BundleMon website ➡️ |
Contributor
|
Documentation Preview: https://kit-docs-bti7z8xxx-anza-tech.vercel.app |
Member
Author
mcintyre94
force-pushed
the
graphite-base/1607
branch
from
f980ae5 to
4085fd5
Compare
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
First PR in the stack adding the React/Kit-client bindings: a ClientProvider, two consumer hooks (useClient, useClientCapability), and a usePromise shim that backports React.use to React 18. Two new errors (SOLANA_ERROR__REACT__MISSING_PROVIDER, SOLANA_ERROR__REACT__MISSING_CAPABILITY) are added in a fresh [9000000-9000999] range. The shape is clean: the provider is a thin value channel, the runtime capability check lives in one place, and the typetests pin the generic narrowing behaviour.
Overall this looks good. One real concern around Rules-of-Hooks in ClientProvider, plus a few smaller polish items below.
Things to watch out for
ClientProvider calls usePromise conditionally
const resolved = isPromiseLike(client) ? usePromise(client) : client;This is a conditional hook call. As long as a given <ClientProvider> instance is consistently fed either a sync client or a promise it works in practice — and the README does ask for a stable reference — but:
- It will trip
react-hooks/rules-of-hooks(assuming the repo's eslint config has the rule enabled, which it typically does for a*.tsxpackage with a React peer dep). - It quietly relies on the consumer never flipping between the two shapes for one provider instance. A toggle that swaps between a precomputed sync client and
createClient().use(asyncPlugin())would crash with a hook-order mismatch — and the README's advice is about reference stability, not sync-vs-async stability.
Two straightforward fixes, either is fine:
- Always go through the unwrap path and have the helper short-circuit on non-thenables, e.g.
usePromise(client)whereusePromisereturnsvaluedirectly when not promise-like (no hook calls at all on that branch is also OK because the call site itself is unconditional now). - Split into two components —
<SyncClientProvider>and<AsyncClientProvider>— and letClientProviderpick one based on the prop shape. Each child has a single, unconditional hook call.
I'd lean toward (1) for simplicity.
No coverage for the rejected-promise path
usePromiseShim re-throws entry.reason on subsequent renders, but there's no test that a rejected client promise surfaces to the nearest error boundary. Worth adding alongside the existing pending/fulfilled cases — both because it's a real path users will hit (failing async plugin) and because it locks in the contract against future refactors of the cache.
Note for subsequent reviewers
- The PR depends on
@solana/plugin-corebeing part of@solana/react's dependency graph. The README examples importcreateClientandClient*types from@solana/kit— ifkitdoesn't yet re-export fromplugin-core, those snippets won't run as-is. Worth a sanity check that the umbrella exports land in the same release window as this PR, or that the docs use@solana/plugin-coredirectly until then. - Two error codes are reserved in a brand-new
[9000000-9000999]range. Per CLAUDE.md these are stable forever — fine here, just calling it out so the range allocation gets a second pair of eyes. usePromise.tsis intentionally not re-exported fromindex.ts, keeping it package-internal. Good.
Smaller polish
Inline below.
| if (missing.length > 0) { | ||
| throw new SolanaError(SOLANA_ERROR__REACT__MISSING_CAPABILITY, { | ||
| capabilities: missing, | ||
| hookName, |
There was a problem hiding this comment.
Minor: (name in client) will return true for inherited properties too. In practice every Kit client is built via Object.defineProperties({}, ...) so it's effectively own-properties only — but if a future plugin ever returns a class instance with capabilities on the prototype, the check would still pass while a Object.hasOwn check wouldn't. Either is defensible; flagging in case the team has a stronger opinion on the intended contract here ("capability declared" vs "capability is an own property").
Member
Author
There was a problem hiding this comment.
Updated to Object.hasOwn - matches how extendClient uses Object.getOwnPropertyDescriptors.
mcintyre94
force-pushed
the
react/provider
branch
2 times, most recently
from
75196f0 to
e7cf86e
Compare
mcintyre94
force-pushed
the
graphite-base/1607
branch
from
4085fd5 to
da868aa
Compare
mcintyre94
force-pushed
the
react/provider
branch
2 times, most recently
from
a9d79e3 to
d073150
Compare
Member
Author
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review — LGTM, all of my prior points have been addressed:
- Rules-of-Hooks:
ClientProvidernow callsusePromise(client)unconditionally, with the runtime check moved insideusePromise. The call site is clean and won't trip the lint rule. - Rejected-promise coverage: new test using
react-error-boundaryexercises the rethrow path inusePromiseShim. Nice. trackedPromisereturns the chained promise: now documented inline with a clear explanation (retry runs strictly after the cache update).Object.hasOwn: switched over, withES2022.Objectadded to the tsconfig lib accordingly.- README: the duplicated suspends-via-Suspense sentence is gone.
One residual subtlety (non-blocking)
usePromise itself still has a conditional React.use call internally: on the sync branch it returns immediately and no hook is invoked, on the async branch it invokes React.use (or the shim). From the caller's perspective (ClientProvider) this is a single unconditional function call, so ESLint is satisfied — but if a given <ClientProvider> instance ever flipped between a sync client and a promise-shape client across renders, hook order would still mismatch.
In practice this can't really happen given the README's stable-reference contract and the natural lifecycle of a client, so I think this is fine as-is. Worth a one-line comment in usePromise noting the assumption ("caller must not flip between sync/async for a given instance") but I wouldn't hold the PR on it.
Notes for subsequent reviewers
- The
useClientoutside-provider test (renderHookwithout a wrapper) will log React's noisy uncaught-error warning. Cosmetic — can be silenced with aconsole.errorspy if it bothers anyone. usePromiseShim's cache is a module-levelWeakMapkeyed by promise identity. Fine for the documented contract, but note that the cache persists for the module lifetime — tests that reuse promise instances across cases would need to be aware. None do today.- Two error codes still occupy the fresh
[9000000-9000999]range. Same flag as before — worth a second pair of eyes on the range allocation.
Adds the Kit client context layer for `@solana/react`: a single provider that publishes a caller-owned Kit client to its subtree, plus two hooks — `useClient` (basic accessor with optional generic narrowing) and `useClientCapability` (runtime-checked accessor with a structured missing-capability error). Required by these hooks and by any plugin-specific hook that depends on a client capability; generic primitives like `useAction` (forthcoming) work against arbitrary async functions and don't need a provider. The provider accepts both synchronous clients and promise-returning ones — when given a promise (e.g. `createClient().use(asyncPlugin())`) it suspends via the nearest `<Suspense>` boundary until the client resolves. On React 19 it delegates to `React.use(promise)`; on React 18 an internal thrown-promise shim, keyed by promise identity, honours the same contract. Two new `SolanaError` codes are reserved in the `[9000000-9000999]` range: `SOLANA_ERROR__REACT__MISSING_PROVIDER` (thrown by `useClient` outside a provider) and `SOLANA_ERROR__REACT__MISSING_CAPABILITY` (thrown by `useClientCapability` with `hookName` + `providerHint` context, listing every missing capability when an array is passed). Additive; no impact on the existing wallet-account hooks. Verified with browser + node unit tests and TS-only typetests across both `@solana/react` and `@solana/errors`.
mcintyre94
force-pushed
the
react/provider
branch
from
d073150 to
f473f42
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of Changes
This PR is the first of a stack of additions to
@solana/reactto provide full react functionality for Kit appsIt adds the
ClientProviderand two hooks to consume it.ClientProviderallows you to expose anyClientto the tree. It supports async clients (ie those using an async plugin), by implementing a shim ofReact.usefor react <19. The client is owned by the app outside of the provider lifecycle.useClient<TClient>is a hook to get that client anywhere in the treeuseClientCapability<TClient>is the same, but with runtime checking for specific capabilities (provided by plugins) and a friendly error message to identify what's missing. This is what plugin hooks should typically use. Eg the wallet plugin would use this to provide functionality that depends on havingclient.walletinstalled.