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

@atproto/api@next integration #7344

Merged
merged 158 commits into from
Feb 18, 2025
Merged

@atproto/api@next integration #7344

merged 158 commits into from
Feb 18, 2025

Conversation

estrattonbailey
Copy link
Member

@estrattonbailey estrattonbailey commented Jan 3, 2025

tl;dr migration steps

  • Calls to validateRecord become asPredicate(*.validateRecord)(record)
    • Optionally use dangerousIsType util created in this PR for faster checks
  • When constructing raw objects for applyWrites and other low level calls, you need to type them with $Typed
  • When constructing records for XRPC agent calls like app.bsky.graph.list.create(), those expect Un$Typed records, so that util is sometimes useful
  • Check out the docs below on new utils for working with profiles and post embeds

TODO

  • test with #main embeds
  • update SDK after publish

This PR integrates bluesky-social/atproto#2999 into our frontend codebase. From that PR:

  • it removes the [x: string]: unknown index signature in objects & records. Because of this, access to un-speccified fields must be explicit.
  • it makes the $type property explicit (though optional, except in open unions). This allows to properly discriminate entity types, especially when used in open unions.
  • the isMyType(value) utilities no longer type cast the value being checked. They only type the presence of the $type property. Use isValidMyType to assert the type through validation.

Those three updates are all covered here.

3

Swapping isType checks with isValidType checks fixes the vast majority of edits in this PR. However, the latter runs full lexical validation of the object. While this is more correct, anything in the critical path obviously should not run validations for performance reasons.

To that end, this PR introduces a dangerousIsType util, which is used like this:

if (AppBskyActorDefs.isProfileView(profile)) {...}
// becomes
if (dangerousIsType<AppBskyActorDefs.ProfileView>(profile, AppBskyActorDefs.isProfileView)) {...}

This util uses the same identity method isProfileView that we've been using, and additionally performs the type cast previously handled by isProfileView itself. Hence why this method requires us to pass in the type we expect.

Though we decided to use the term "dangerous", this isn't terribly dangerous. We should be able to trust our API responses to return only validated objects, see Slack convo.

Note

In cases where the validation check is not in the critical perf path, I did chose to use the more simple isValid* utils. There are a few such cases in this PR.

1 & 2

This is the meatier change, and these two overlap a lot in practice.

As mentioned in the atproto repo PR, our old types didn't have the $type property defined (see how ProfileViewBasic is defined here). This made understanding these types somewhat confusing, and prevented directly checking the $type prop to disciminate these open unions e.g. if (view.$type === 'app.bsky.feed.post') { ...handle post type... }. With this change, we can now do this.

Worth noting, these open unions still require us to provide a trailing unknown type to accommodate future iteration. This means that we need to strictly discriminate this union to get proper types e.g. we need to check each case of the union, otherwise TypeScript will assert that the object is of type $Typed<{ $type: string }>.

This mostly affects the frontend codebase when we're dealing with profile views (and to a lesser extend starter pack views) of which we have a few with varying levels of detail. Prior to this, ProfileViewBasic ProfileView and ProfileViewDetailed (as well as the additional ChatBskyActorDefs.ProfileViewBasic) were all loosely equal, and we unfortunately used them interchangeably in a number of places throughout this codebase.

In many cases, this is by design. UserAvatar is used in many places, and only relies on props available on all profile view types. FollowButton is another example. Many of our queries behave like this too.

One option here would be to refactor all these cases so that these components only accept those props they need i.e. pass in profile.did only instead of profile in its entirety. That would require much more work, so I didn't take that approach here. It may be a good idea to consider doing this in the future.

Another would be to use omit the $type param entirely like Omit<AppBskyFeedDefs.PostView, '$type'>, but then we aren't even sure what types are coming through and we lose the ability to discriminate types.

The option I decided to move forward with was creating an abstraction of these types called AnyProfileView, that is simply a union of all 4 profile types we use in the app. This single source of truth saves us having to create these unions in dozens of places.

export type AnyProfileView =
  | AppBskyActorDefs.ProfileViewBasic
  | AppBskyActorDefs.ProfileView
  | AppBskyActorDefs.ProfileViewDetailed
  | ChatBskyActorDefs.ProfileViewBasic

This util is intended to be used in these component API contracts. Within that, we can discriminate the union to determine what can actually be rendered. See an example of how I did this in the old ProfileCard.

Note

When referencing a property typed as AnyProfileView, accessing common props like did shows no feedback. But accessing description will emit a type error, since it's not available on ProfileViewBasic. So we're still forced to use the identity utils when checking for potentially missing values.

Lastly, when constructing records manually on the frontend, like when creating reports, we now need to use the included $Typed util, which asserts that $type is in fact defined. This made a few things a little more verbose, but safer in the long run. Example here.

New concepts

This PR introduces the following concepts.

#/types/atproto

This is intended to be commonly used, and imported like below. I'll just list out the full APIs here:

import * as atp from '#/types/atproto'

atp.dangerousIsType<DesiredType>(object, isType()): boolean

// profiles
type atp.profile.AnyProfileView
atp.profile.isBasicView(object): boolean // is `ProfileViewBasic`
atp.profile.isView(object): boolean // is `ProfileView`
atp.profile.isDetailedView(object): boolean // is `ProfileViewDetailed`

// starter packs
type atp.starterPack.AnyStarterPackView
atp.starterPack.isBasicView(object): boolean // is `StarterPackViewBasic`
atp.starterPack.isView(object): boolean // is `StarterPackView`

// post
atp.post.parseEmbed(post.embed)

Note: I aliased the type utils for convenience, but that's totally optional.

atp.post.parseEmbed

Complexity around embed rendering has plagued us for a while. With this new update, we can now do exhaustive union checking, which is great, but verbose. Instead importing AppBsky*Defs and identity functions and doing the ~13 type checks required (some nested) in the handful of places in the app where we render post embeds, this util aims to make this handling more ergonomic by reconstructing the lexicon's open union into a new discriminated union shape.

You can see a full example here. Usage looks like this:

const embed = atp.post.parseEmbed(post.embed) // where embed is AppBskyFeedDefs.PostView['embed']

if (embed.type === 'post') {
  // embed is type AppBskyEmbedRecord.ViewRecord
  const count = embed.view.replyCount
} else if (embed.type === 'images) {
  // embed is type AppBskyEmbedImages.View
  const images = embed.view.images
}

Unstable profile cache

We've been using a kinda hacky "profile cache" via precacheProfile for a while now that pre-fills profile data when clicking on profile links. I say hacky because we're using React Query's client cache, without ever actually using the query key in a useQuery call, or ever referencing this data from the profile shadow.

Example of how we've used this:

  • PreviewableUserAvatar is used in multiple components, right. And those components may pass in any view type and thereby pass that view type into PreviewableUserAvatar
  • ProfileCard is one example of this. ProfileCard can be used with multiple view types, though it’s mostly ProfileViewBasic
  • NotificationFeedItem is another example. Here, the author view is actually ProfileView
  • PreviewableUserAvatar calls precacheProfile on press to provide data to placeholderData on the profile query so that we can render the profile page more quickly

So you see: precacheProfile has — for a long time — been handling varied profile view types. In fact, in resolve-uri.ts we were casting it as ProfileViewBasic (source) and in profile.ts we were casting it as ProfileViewDetailed (source) even though the data returned is probably never a Detailed view (haven’t verified this, just a hunch).

So this PR introduces a new interface for "unstable profile cache" located in #/state/queries/unstable-profile-cache. See the file for full details, should be pretty clear.

Misc

  • In some cases, TS simply notified us that we were actually just using the wrong profile view. Like, changing ProfileView to ProfileViewBasic was sufficient to quiet errors, and was correct based on the data that was being passed around.
  • In some cases, it's enough to check <prop> in object to sufficiently discriminate unions (example)

Copy link

github-actions bot commented Jan 3, 2025

Old size New size Diff
6.93 MB 6.93 MB 0 B (0.00%)

@@ -360,7 +359,7 @@ function responseToThreadNodes(
depth,
isHighlightedPost: depth === 0,
hasMore:
direction === 'down' && !node.replies?.length && !!node.replyCount,
direction === 'down' && !node.replies?.length && !!post.replyCount,
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this was actually a typo, which was masked by the [key: string]: undefined loose types we use to have here. node does not have a replyCount on it at all, but node.post does.

@arcalinea arcalinea temporarily deployed to next/base - social-app PR #7344 February 11, 2025 16:17 — with Render Destroyed
@arcalinea arcalinea temporarily deployed to next/base - social-app PR #7344 February 11, 2025 16:27 — with Render Destroyed
* origin/main: (39 commits)
  Revert "fix quote & feed padding not being pressable (#7694)" (#7733)
  Nightly source-language update
  Add dev mode for easy copying of at:// URIs and DIDs (#7723)
  Improved search language select (#7591)
  Improved search page (#7590)
  Animate drawer menu on mobile web (#7711)
  Special treatment for recommended starter packs (#7706)
  invert new postonboarding gate (#7695)
  Animate dropdown menus (#7704)
  Better animations for dialogs, animate web composer (#7703)
  fix quote & feed padding not being pressable (#7694)
  ungate trending (#7696)
  run prettier (#7727)
  Nightly source-language update
  Add deploy key to nightly commit (#7722)
  [APP-1031] Add new followerRule to threadgate settings (#7681)
  Fix bug in GH action nightly-update-source-languages.yaml
  More i18n process updates (#7720)
  Run intl:extract on english to see if crowdin picks up on the changes
  Refine accessibilityHint (#7554)
  ...
@arcalinea arcalinea temporarily deployed to next/base - social-app PR #7344 February 13, 2025 18:30 — with Render Destroyed
@arcalinea arcalinea temporarily deployed to next/base - social-app PR #7344 February 13, 2025 18:48 — with Render Destroyed
* origin/main:
  Make Android app start faster by disabling JS bundle compression (#7751)
  Nightly source-language update
  Update tests
  Screen for searching user's posts (#7622)
  Add translations missed in last PR (#7748)
  1.98 release: Pull latest from crowdin (#7746)
  [Instrumentation] Signin (#7742)
  Reenable router events (#7735)
  Nightly source-language update
  Bitdrift integration (#7728)
  Use effective filtering for feeds (#7736)
  Update PostInteractionSettingsDialog.tsx (#7726)
* origin/main:
  Fix "log in", "log out" to "sign in", "sign out" (#7739)
@arcalinea arcalinea temporarily deployed to next/base - social-app PR #7344 February 17, 2025 17:02 — with Render Destroyed
Copy link
Member

@mozzius mozzius left a comment

Choose a reason for hiding this comment

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

💎

The new utilities look great. Let's get it in so we can test

@estrattonbailey estrattonbailey merged commit a51fc8e into main Feb 18, 2025
6 of 7 checks passed
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.

4 participants