-
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
add disconnect method to page api #1200
Conversation
🦋 Changeset detectedLatest commit: 1ec72d2 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 |
33ec5f9
to
a926e9e
Compare
apps/extension/src/content-scripts/injected-disconnect-listener.ts
Outdated
Show resolved
Hide resolved
class PraxInjection { | ||
private static singleton?: PraxInjection = new PraxInjection(); | ||
|
||
public static get penumbra() { | ||
return PraxInjection.singleton!.injection; | ||
} |
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.
I was having a hard time following the lifecycle of the singleton. Think we can do:
class PraxInjection {
private static singleton?: PraxInjection;
public static get penumbra() {
if (!PraxInjection.singleton) {
PraxInjection.singleton = new PraxInjection();
}
return PraxInjection.singleton.injection;
}
and get rid of this in the constructor:
if (PraxInjection.singleton) return PraxInjection.singleton;
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.
we want listener attach and injection to happen as early as possible. having the class statically init an instance as soon as it's in scope achieves that
a constructor that prefers to return an existing singleton prevents a duplicated singleton. i changed get penumbra
to use this behavior instead of an assert.
disconnect: () => this.endConnection(), | ||
connect: () => (this.state() !== false ? this._connect.promise : this.connectionFailure), | ||
isConnected: () => this.state(), | ||
request: () => this.postRequest(), |
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.
What does request do? I think the PenumbraInjection
type needs some annotations for the usages for each method.
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.
this is the same client api we had before.
request is a method that initiates a request for origin approval
adds disconnect method to page api
page injection is refactored as a class to more explicitly represent and control injection state
adds listeners in prax to handle disconnect
for some reason, connection stalls on the ibc page, but works everywhere else.
minifront ui in subsequent pr