Lexicon: embeds in record embeds #691

devinivy · 2023-03-18T20:39:49Z

A few changes to record embeds in here:

Embed view lexicons are named as #view rather than #presented, for consistency.
Embed views now have a standard wrapper shape { $type, value }, whereas before we had different keys { $type, record }, { $type, external }, etc. That wasn't necessary since $type acts as a discriminator for the wrapped value. One small upside there is that the record under a record embed is now (embed.value).record rather than (embed.record).record, which feels a little nicer to write.
Record embed views now contain embeds which can include one or more views of embeds found within the record. If there are multiple, it would be up to the consumer to distinguish them by inspecting embed.value.record. Embeds are only hydrated one level deep.
Fixes to external embeds: the uri field was marked as an at-uri, whereas it's actually not limited to at:// (most likely will be a web uri https://).

Resolves #619

dholms · 2023-03-18T21:32:54Z

lexicons/app/bsky/embed/external.json

@@ -17,7 +17,7 @@
      "type": "object",
      "required":  ["uri", "title", "description"],
      "properties": {
-        "uri": {"type": "string", "format": "at-uri"},
+        "uri": {"type": "string"},


i snuck in the uri format btw 👀

dholms · 2023-03-18T21:37:02Z

lexicons/app/bsky/embed/record.json

      "properties": {
        "uri": {"type": "string", "format": "at-uri"},
        "cid": {"type": "string", "format": "cid"},
        "author": {"type": "ref", "ref": "app.bsky.actor.defs#withInfo"},
-        "record": {"type": "unknown"}
+        "record": {"type": "unknown"},
+        "embeds": {


is this an array or just a union?

A single record only has one embed right?

A post record has at most one embed, but if we want this to remain generic for any record type, I figured there's nothing preventing a record from containing multiple embeds.

If there are multiple, it would be up to the consumer to distinguish them by inspecting embed.value.record.

Hmmm i think im inclined to make it only one embed 🤔

If you want multiple embeds then you define a new type for that in the union. Similar to how we handle images which is actually up to 4 embeds but represented as only a single embed

This let's us set constraints on how many objects of what type are embedded instead of an unconstrained array

Thoughts?

I guess that also only works if this union is only for schemas that we control. because another developer could take an alternate approach and have some record contain an array of embeds. But I suppose then we could just define a new single embed type that points to that array of embeds. 🤔

I see what you're getting at 👍 I do think I still lean towards the array but I'm also definitely not married to it. Part of my view on it is treating embeds the same way we might treat any other type: if record embed views needed to present all blobs that appear in the record, for example, I imagine we might end-up with an array of blobs here. That would be necessary to cover cases like profile records which have multiple blobs, i.e. both an avatar and a background banner. I was imagining a similar case where a record could conceivably contain two or more separate embed fields, since there's nothing restricting that.

Here's a case to illustrate where my head was at! Consider a "question and answer" record, where an actor can present a post that poses a question next to a post that answers that question. It might be modeled like this, with an embed for the question post and another for the answer post:

{ "commentary": {"type": "string"}, "questionEmbed": {"type": "ref", "ref": "app.bsky.embed.record"}, "answerEmbed": {"type": "ref", "ref": "app.bsky.embed.record"} }

At the same time I'd be totally cool saying that we're just not going to cater to that for record embeds, that there's a convention that you should avoid creating records with multiple embed fields 👍 We also do always have the option to go back on the idea of this being a generic record embed, and instead making it specifically a post embed.

The argument would be that the presentation of that possibility would be

"questionAndAnswerEmbedView": { "questionEmbed": {"type": "ref", "ref": "app.bsky.embed.record"}, "answerEmbed": {"type": "ref", "ref": "app.bsky.embed.record"} }

And similarly any array of heterogenous embeds can be encapsulated in a lex type that boils it down to a single value that's expressive about what exactly is in the array.

With that being said, it starts to sound a bit overly complex/galaxy brained as a way of encoding what's essentially a list of supplemental "view" data

I think I'd feel more strongly if this was primary data. But since it's supplemental data & the actual structure is imposed by the record, I'm game to leave this field relatively unstructured & just return an array of heterogenous embeds.

Cool cool 👍 it's just a little tricky to deal with view information for any record in a unified way. In the case of a complex record like the question-and-answer one, I would expect a consumer to discover embeds in the original record data. And then fish the view information for those embeds out of this array.

I imagine same might go for blobs if we introduce those here: you discover a blob in the original record, and then fish the view of that blob (e.g. a url to a resized image) out of some collection/array of blob views.

yup yup that checks out. im game with just the array here. thx for the back and forth 🙏

dholms · 2023-03-18T21:42:50Z

packages/pds/src/app-view/services/feed/index.ts

+      this.embedsForPosts(
+        records.map((p) => p.uri),
+        requester,
+        _depth + 1,


does this go arbitrarily deep?

so like in the case of a quote post of a quote post of a quote post of an image, this'll return that image?

oh wait just saw the _depth > 1 check. so only checks one deep?

Exactly, there's a depth check in two places to ensure embeds are only included two levels deep (i.e. you can receive an embed's embed):

At the top of the function to ensure it doesn't recurse too deep.

When mapping to embeds in the view. The purpose there is to distinguish embeds being missing ("embeds are not included" e.g. due to depth) versus empty [] ("embeds are included but there are none").

dholms · 2023-03-18T21:53:27Z

packages/pds/src/app-view/services/feed/index.ts

                uri: formatted.uri,
                cid: formatted.cid,
                author: formatted.author,
                record: formatted.record,
+                embeds: formatted.embed


this is one too many ternaries for my tired saturday brain to handle haha 🙃

I don't quite understand the difference between undefined & the empty array here?

& similar to my prev comment, is there ever a case when the array contains more than one value?

Just dropped a couple comments above that will hopefully make it clearer what I was going for. However we land on it, I'll clean this up and drop some comments in the code 👍

dholms

💎

devinivy added 2 commits March 18, 2023 14:37

Update format of post embed views, fix external uri validation

5a16061

Include embeds on record embeds

161c0b2

devinivy mentioned this pull request Mar 18, 2023

Lexicon: notify users when they are quoted #692

Merged

dholms reviewed Mar 18, 2023

View reviewed changes

devinivy mentioned this pull request Mar 18, 2023

Lexicon refactor #658

Merged

Tidy/clarify deep embeds

473306f

dholms approved these changes Mar 19, 2023

View reviewed changes

devinivy merged commit 92ecee7 into lex-refactor Mar 19, 2023

devinivy deleted the lex/embeds-in-embeds branch March 19, 2023 23:50

devinivy mentioned this pull request Mar 23, 2023

Lexicon: support separate images embed #708

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lexicon: embeds in record embeds #691

Lexicon: embeds in record embeds #691

devinivy commented Mar 18, 2023 •

edited

Loading

dholms Mar 18, 2023

dholms Mar 18, 2023

dholms Mar 18, 2023

devinivy Mar 18, 2023 •

edited

Loading

dholms Mar 18, 2023

dholms Mar 18, 2023

devinivy Mar 19, 2023

dholms Mar 19, 2023

dholms Mar 19, 2023

devinivy Mar 19, 2023 •

edited

Loading

dholms Mar 19, 2023

dholms Mar 18, 2023

dholms Mar 18, 2023

devinivy Mar 18, 2023 •

edited

Loading

dholms Mar 18, 2023

devinivy Mar 18, 2023

dholms left a comment

Lexicon: embeds in record embeds #691

Lexicon: embeds in record embeds #691

Conversation

devinivy commented Mar 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinivy Mar 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinivy Mar 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinivy Mar 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholms left a comment

Choose a reason for hiding this comment

devinivy commented Mar 18, 2023 •

edited

Loading

devinivy Mar 18, 2023 •

edited

Loading

devinivy Mar 19, 2023 •

edited

Loading

devinivy Mar 18, 2023 •

edited

Loading