adr 006 changes and implementation

[turbocrime]

meeting suggestions in #1647

suggestions available in adr doc in this pr

still removes react package, it would be broken by implementation

currently

implements PenumbraClient as a class with static and instance methods.

• static methods support inspection of available providers to support making a choice of provider
• a class instance is created to encapsulate client configuration and state
  • provider selection
  • transport config
  • connection and connection state
  • callbacks

a class instance may be created without configuration, but must be configured with provider to connect.

an unconfigured client instance

• can select a provider by parameter of its .connect method
• supports adding client connection state change callbacks
• can't forward the provider's EventTarget methods (no provider to forward)
• returns 'undefined' from most other methods depending on a configured state
• will async reject connection to an undefined provider
• sync throw use of services on an undefined provider

questions remaining

should an unconfigured client instance which possesses callbacks activate the callbacks immediately when a provider is configured with an event representing the new provider's present state, or wait until the provider emits a state change event?

should a callback added to a client be immediately activated with an event representing the present state, or wait until the provider emits a state change event?

should an existing unconfigured client instance provide a feature to select a provider without attempting connection?

is it appropriate to throw creation of a service client when no provider is configured?

is it appropriate to implicitly request a connection if a service client is created when a provider is configured but not connected?

i'd like to add

when parsing the manifest, the client could either reformat the icon URLs to a fully-qualified path, or simply fetch them so they are available synchronously.

[changeset-bot]

🦋 Changeset detected

Latest commit: 4a95047

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@penumbra-zone/client	Major
minifront	Patch

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

[VanishMax]

We are extremely close! I think that the requested changes are final

[VanishMax]

@turbocrime regarding the questions in the PR description, sharing my opinion:

should an unconfigured client instance which possesses callbacks activate the callbacks immediately when a provider is configured with an event representing the new provider's present state, or wait until the provider emits a state change event?

no need for activating them immediately – users can always check the state an any time.

should a callback added to a client be immediately activated with an event representing the present state, or wait until the provider emits a state change event?

is it different from the previous question? don't really see the difference, so the answer remains

should an existing unconfigured client instance provide a feature to select a provider without attempting connection?

this could be useful – will save users at least one function call

is it appropriate to throw creation of a service client when no provider is configured?

probably no

is it appropriate to implicitly request a connection if a service client is created when a provider is configured but not connected?

this might be an unexpected UX. Maybe in the future we'll add some more wrappers or handlers to the service calls with some logic but I'd leave it as-is for now

i'd like to add: when parsing the manifest, the client could either reformat the icon URLs to a fully-qualified path, or simply fetch them so they are available synchronously.

+1 on this, could be here or we can create an issue for this and leave for later

[turbocrime]

should an unconfigured client instance which possesses callbacks activate the callbacks immediately when a provider is configured with an event representing the new provider's present state, or wait until the provider emits a state change event?

no need for activating them immediately – users can always check the state an any time.

the purpose of a callback is to notify of state changes, but there are two locations with state- the client, and the provider. for instance consider this pattern:

1. a provider becomes connected on page init
2. an unconfigured client is created
3. a callback is attached to the unconfigured client
4. the client is configured to associate with the already-connected provider

now, the client has transitioned from a 'connected = undefined' state to a 'connected = true' state, but no callbacks were activated.

the client is not explicitly holding an active port, but when queried will report its provider's 'true' connected status. its provider is connected, and no event has been emitted, and no transport has been constructed.

should a callback added to a client be immediately activated with an event representing the present state, or wait until the provider emits a state change event?

is it different from the previous question? don't really see the difference, so the answer remains

it is different from the previous question by the sequence of operations: callback is attached after event. consider this sequence

1. a provider is available on page init
2. a client is created, configured or unconfigured
3. the client connects
4. a callback is added to the client, hoping to notice a connected state

the callback added in 4 will likely never fire. i believe this is widely understood, and ok, but trying to confirm all details.

should an existing unconfigured client instance provide a feature to select a provider without attempting connection?

this could be useful – will save users at least one function call

ok, will expose existing private method that performs this function. this does not save any calls, but rather provides the opportunity to conduct more function calls, and it adds another ambiguous/intermediate state.

is it appropriate to throw creation of a service client when no provider is configured?

probably no

the alternatives are

1. the return type of the service function must include undefined, which must be guarded by every caller, and which seems to break intended use patterns.
2. the service function must make a connection request and return a client while that request is pending
3. the service function must be async, to make a connection request and await the connection state

if the undefined behavior is implemented, then it opens the question: should calling connect with an unconfigured provider reject? how would such a connection failure be distinguished from a connection success by the caller?

is it appropriate to implicitly request a connection if a service client is created when a provider is configured but not connected?

this might be an unexpected UX. Maybe in the future we'll add some more wrappers or handlers to the service calls with some logic but I'd leave it as-is for now

ok, this will necessarily return undefined, and restricts to option (1) above. the current implementation, described in the comment on the service function which you gave 👍 , makes the implicit request and chooses option (2).

i'd like to add: when parsing the manifest, the client could either reformat the icon URLs to a fully-qualified path, or simply fetch them so they are available synchronously.

+1 on this, could be here or we can create an issue for this and leave for later

ok

[VanishMax]

@turbocrime

• is it appropriate to throw creation of a service client when no provider is configured?
• probably no
• the alternative is the return type of the service function must include undefined, which will be indistinguishable from the next case. is this acceptable? should calling connect on an unconfigured provider match this behavior?

but we can create a PromiseClient that will return empty port when the client is not configured, aren't we? This way there's no need to return undefined or throw a on service creation – instead the service method call with throw.

as for the other questions (about callbacks, for example) i don't have strong opinions, so i'd be ok with any implementation

[turbocrime]

but we can create a PromiseClient that will return empty port when the client is not configured, aren't we? This way there's no need to return undefined or throw a on service creation – instead the service method call with throw.

this is an option 4. which i avoided because it is likely confusing to the consumer

• why deliberately allow creation of a broken client?
• will consumers understand they need to recreate the client?
• consumers are now likely to attempt techniques containing race conditions
• the failure mode of the client is a timeout, which is obscures the cause

[turbocrime]

at some point, if the caller is doing something wrong, we have to throw.

it is better to throw at the location of the problem, when the problem is noticed. creating broken outputs like that is essentially a null propagation error. we should enable developers to use this library correctly by rejecting conditions that are incorrect.

i dislike that 'void' or 'undefined' are acceptable returns from many of these functions. it seemed very desired to we avoid things like 'undefined' return, but they've been added back now in response to discussion.

simply returning 'undefined', when there is an explainable problem, obscures what's happening and is ultimately confusing rather than robust.

[VanishMax]

@turbocrime alright, let's throw an error from the service creation. If one day we find a more neat solution to change the behavior, it won't be a breaking change

[turbocrime]

with the addition of this check,

web/packages/client/src/client.ts

Lines 328 to 333 in 52e7c84

	const request = confirmManifest.then(async () => {
	if (isLegacyProvider(provider)) {
	await provider.request();
	}
	return provider.connect();
	});

and the removal of the event type change,

c39c502

prax 11.13.1 is functional with all features of this ADR.

prax must still be updated to support more than one connection within a document lifecycle, but that work is in prax-wallet/prax#134

[turbocrime]

questions answered

should an unconfigured client instance which possesses callbacks activate the callbacks immediately when a provider is configured with an event representing the new provider's present state, or wait until the provider emits a state change event?

yes. an unconfigured client with callbacks will notify listeners when it attaches to a provider. this is how you expected it to function in your nextjs demo.

should a callback added to a client be immediately activated with an event representing the present state, or wait until the provider emits a state change event?

no. a caller who wishes to identify the present state must do so by checking the present state.

should an existing unconfigured client instance provide a feature to select a provider without attempting connection?

this is provided and termed attach, but it seems fairly useless. theoretically, one may maximally exercise this feature to

1. create an unconfigured client
2. add a callback
3. manually attach the client to a provider without initiating connection
4. receive the provider's potentially non-connected state in the callback, without causing effect
5. ...etc

Given that the connect method now uses attach internally, this flow may be common, and is demonstrated in your nextjs example:

1. create an unconfigured client
2. add a callback
3. manually connect the client, specifying a provider origin parameter
4. the client attaches. receive the provider's present state in the callback
5. if the provider was not connected, the provider begins connecting. if this is a new state, receive the provider's pending state in the callback
6. ...etc

is it appropriate to throw creation of a service client when no provider is configured?

creating a service client before attaching to a provider will throw.

this will prevent nonsensical use patterns, or use patterns creating subtle timeout issues or race conditions with configuration.

notably: consumers should not use an unconfigured client to create a service client at a top-level scope and then export it.

is it appropriate to implicitly request a connection if a service client is created when a provider is configured but not connected?

• if the client is attached and reports connected === true, then it will construct a transport if necessary and return a client expected to work
• if the client is attached and reports connected === false, then it will throw a connection error specifying its attached origin if creation of a service client is attempted
• if a client is not attached, it reports connected === undefined and it will throw a connection error specifying an undefined origin if creation of a service client is attempted

[turbocrime]

notably, as it was requested that operations on a client with no configured provider should return undefined instead of throwing where reasonable, this implementation involves a field indicating a three-state true/false/undefined connection status, with undefined representing the connection state of a client which is not attached to a provider. i just want to point that out.

[VanishMax]

🎉🎉🎉