-
Notifications
You must be signed in to change notification settings - Fork 17
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
react provider 3 #1450
react provider 3 #1450
Conversation
🦋 Changeset detectedLatest commit: 87d69ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
4a820a2
to
b99c577
Compare
d04ddce
to
d46337b
Compare
d46337b
to
a219aea
Compare
cc @grod220 |
0670bce
to
fb9404f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work! This is really great. I'm excited to see it in use.
I left a lot of comments, but they're almost all very minor things. Feel free to self-merge after addressing.
packages/react/README.md
Outdated
|
||
## Possible provider states | ||
|
||
On the bare Penumbra injection, there is only a boolean/undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should explain what the "Penumbra injection" is? i.e., that it's an object injected by the extension (and perhaps link to the TypeScript interface — I forget where it is)?
Again, feel free to ignore if this is already obvious to web3/dapp devs
} | ||
}, [failure, penumbra, port, resolvePort, rejectPort, state]); | ||
|
||
return transport; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the transport automatically queue up requests until the connection is ready? Or how is the queueing handled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
behavior is in https://github.com/penumbra-zone/web/blob/main/packages/transport-dom/src/create.ts
each request awaits a shared connection attempt as part of its lifecycle, involving the getPort
option. the connection respects the timeout configuration passed into the transport.
the difference between the two methods is that
usePenumbraTransportSync
will create a transport, and the transport will await a port with its timeout clock beginning at the first request.usePenumbraTransport
will promise a transport, but won't init the transport until a port appears in context. the transport will spend no timeout clock, but if the port never appears the wait may be indefinite.
} | ||
}, [makeApprovalRequest, providerState, providerInjection?.connect, manifest, failure]); | ||
|
||
const createdContext: PenumbraContext = useMemo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to have context? It would be nice if the consumer wouldn't need to wrap their app with a provider. Especially since it appears that this initiates actions automatically. Doesn't all state live within the injection now? Aka, can't we just rely upon a usePenumbra()
hook without a provider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the developer just wants to grab a client wherever they are, @penumbra-zone/client
already provides methods for that.
import { createPenumbraClient } from '@penumbra-zone/client/create';
import { ViewService } from '@penumbra-zone/protobuf';
// this goes through the same connection process
const viewClient = createPenumbraClient(ViewService, someOptionalOrigin);
export const ChainIdComponent = () => {
const [chainId, setChainId] = useState<string>();
useEffect(() => { // or useQuery or whatever
const { parameters } = await viewClient.appParameters({});
return setChainId(parameters?.chainId);
});
return <div>{chainId}</div>
};
i agree that a dedicated react package isn't much of a value-add. at best this may help prevent a naive developer from constructing/deconstructing a transport and client for every component on every rerender.
yes the injection tracks its own detailed connection state including called methods, so now the caller doesn't have to remember what they've done. but all that it delivers is a MessagePort
. the page still needs to construct its own transport etc.
and if the goal is to share state among components, context providers are the way to achieve shared state in react, unless we expect users to prop-drill the clients everywhere. and it's convenient to throw in things like prefetched manifest data at this level.
fb9404f
to
87d69ef
Compare
implements
<PenumbraContextProvider>
wrapper componentusePenumbra
which acquires context from componentusePenumbraTransport
andusePenumbraService
PenumbraContextProvider
verifies presence of an extension before forwarding context, and provides manifest data synchronously.uses listener interface, so requires prax-wallet/prax#92
check out the readme https://github.com/penumbra-zone/web/blob/09d052686de21d1301c8eca3b712aa2517a0be14/packages/react/README.md