Skip to content
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

feat!: getExtensions isn't a promise #11

Merged
merged 3 commits into from
May 3, 2023
Merged

Conversation

weichweich
Copy link
Contributor

@weichweich weichweich commented May 3, 2023

related https://github.com/KILTprotocol/ticket/issues/2126

This PR changes the getExtension function.

  • remove any side effects (setting the window.kilt property)
  • make it synchronous (makes it more ergonomic to use since it's usable in both sync and async environments)

Future additions

we already dispatch the kilt-dapp#initialized even though it's not part of the Credential API 3.0.

Once the events are part of the credential API we should include a function for listening to new extensions.

@weichweich weichweich marked this pull request as ready for review May 3, 2023 11:14
@weichweich weichweich requested a review from arty-name May 3, 2023 11:30
Comment on lines 19 to 21
// copy all extensions into a new object since the caller should be allowed to change the object
// without changing the underlying extension object.
return { ...apiWindow.kilt }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am surprised to see this requirement. This will also likely lose the very special versions property. I’d rather simply return the kilt object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, I changed it to an array. 0ea1196 and f962370

* If an extension injects itself only after this function is called, it will not be contained in the returned extensions.
* @returns an object containing extensions
*/
export function getExtensions(): Record<string, InjectedWindowProvider<PubSubSessionV1 | PubSubSessionV2>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get a type that includes all the PubSubSessions? That would also be interesting to export as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are referring to all started sessions with the extensions?

it's expected to get a list of extensions
@weichweich weichweich merged commit 1511d70 into main May 3, 2023
@weichweich weichweich deleted the aw-rework-get-extension branch May 3, 2023 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants