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

remove legacy mode and bind api to emitter #83

Merged
merged 2 commits into from
Nov 21, 2023
Merged

Conversation

staltz
Copy link
Member

@staltz staltz commented Nov 20, 2023

Context

In secret-stack I recently added support for more identifiers so that we can different kinds of IDs depending on the muxrpc transformer used (secret-handshake? no-auth? secret-handshake-extended? etc)

Problem

muxrpc still used

createLocalApi(localApi, localManifest, perms).bind(context)

where "context" is { _emit, id } which means the id is hard-coded. This id comes from secret-stack and then is available for reading as this.id where the this is referring to the remote peer. Concrete use case here.

The problem is that with diverse IDs, this.shse.pubkey is not available, because the context object doesn't have that information. We need to bind to the emitter instead:

createLocalApi(localApi, localManifest, perms).bind(emitter)

Solution

I started by binding to the emitter, and that made the context useless (unused anywhere) which in turn led me to remove the legacy mode and some of the index.js, which in turn led to adapting all the tests (they all assumed the legacy mode), that's why this PR looks big. It's all a domino effect from binding to the emitter.

This PR is a breaking change and should be published as a new major version.

Tests

All tests passed when I ran them locally. Standard linting also passed.

@staltz staltz requested review from mixmix and mycognosist November 20, 2023 08:41
Copy link
Member

@mycognosist mycognosist left a comment

Choose a reason for hiding this comment

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

I've not personally used this part of the JS stack but from reading the code changes and tests it looks good to me 🟢

Maybe a README update is in order, since the function signature of createMuxrpc has changed?

https://github.com/ssbc/muxrpc#api-createmuxrpc-remotemanifest-localmanifest-localapi-id-perms-codec-legacy--rpc

@staltz
Copy link
Member Author

staltz commented Nov 20, 2023

Oh, great point, I gotta update that. Thanks!

@staltz
Copy link
Member Author

staltz commented Nov 21, 2023

Updated the readme now :)

@staltz staltz requested a review from mycognosist November 21, 2023 08:37
Copy link
Member

@mycognosist mycognosist left a comment

Choose a reason for hiding this comment

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

Beautiful! Great work 🟢

@staltz staltz merged commit 621ad5d into main Nov 21, 2023
5 checks passed
@staltz staltz deleted the staltz/remove-legacy branch November 21, 2023 10:15
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.

2 participants