-
Notifications
You must be signed in to change notification settings - Fork 554
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
Instantiate XrpcClient from an OAuthAgent #2714
Conversation
2b7b3b5
to
8d1a50e
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.
Looks good! I support this change to the oauth client 👍 I think it will be much friendlier to folks building their own apps, and at little cost of convenience.
It does stand out to me that the dependency of @atproto/oauth-client
on @atproto/api
is now very shallow. I can't help but wonder if there's a way to maintain the convenience of obtaining an OAuthAtpClient
without the hard dependency on @atproto/api
.
Hey @devinivy, thank you for the review. I moved the With this change, the |
This whole PR introduces a breaking change in the cc @foysalit |
Co-authored-by: surfdude29 <149612116+surfdude29@users.noreply.github.com>
Co-authored-by: surfdude29 <149612116+surfdude29@users.noreply.github.com>
Co-authored-by: surfdude29 <149612116+surfdude29@users.noreply.github.com>
Co-authored-by: surfdude29 <149612116+surfdude29@users.noreply.github.com>
Thanks @surfdude29 for your careful review <3 |
No worries, happy to help :) |
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.
A couple tiny questions but this looks good— very excited by these changes. Breaking the dependency between @atproto/api
and @atproto/oauth-client
is also 💎.
``` | ||
|
||
### API calls | ||
|
||
The agent includes methods for many common operations, including: | ||
|
||
```typescript | ||
// The DID of the user currently authenticated (or undefined) | ||
agent.did | ||
agent.accountDid // Throws if the user is not authenticated |
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.
Interesting! I just noticed this part of the api for the first time.
Small thing: I wonder if this should be assertDid
instead of accountDid
to make this clearer?
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.
yep that would probably have been a better name -_-
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'd recommend we add assertDid
and deprecate accountDid
at some point soon.
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.
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.
Really great. I'm excited to be making progress on cleaning up all these interfaces -- they've been clunky for a long time before oauth.
@atproto/api
'sAgent
class can now be instantiated using aSessionManager
interface (new Agent(sessMgr)
)@atproto/client
'sOAuthSession
implements theSessionManager
interface meaning that we can donew Agent(oauthSession)
@atproto/api
'sAtpAgent
works exactly as before. A comment was added to say that is will soon become deprecated as it adds no value anymore.@atproto/api
now exposes anCredentialSession
class that can be used to instantiate bothnew Agent(credSession)
as-well-asnew XrpcClient(credSession, myLex)
Potentially breaking changes:
Agent
is no longer abstract, and its constructor signature has changed. This means that any implementation extending that class will be broken. Since this abstract class was release very recently, the impact should be minimal.Agent
no longer extendsAtpBaseClient
(this is an implementation detail so should no really impact anything)@atproto/oauth-client*
no longer expose anagent
, but asession
instead. Adapting to this is very straight forward (new Agent(session)
).OAuthAgent
was renamed intoOAuthSession
and some method signatures where changed