Integrate lex changes into subscribeRepos route #703
Conversation
dholms
force-pushed
the
subscribe-repos-lex
branch
from
2d42d4d to
6e96a31
Compare
dholms
changed the title
| { msg }, | ||
| 'unexpected message on repo subscription stream', | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Arguably we should move the cursor forward in this situation. But not terribly worried about it, since this area will get more attention soon.
There was a problem hiding this comment.
yeah you're right that we should ideally. but I think I'll hold off on it until we start actually sending/processing those messages. there's not any way for this to currently get into a bad state
| count: i, | ||
| } | ||
| } | ||
| yield { $type: 'io.example.stream2#done' } |
There was a problem hiding this comment.
What should the behavior be for this? I imagine it might end-up in the header discriminated as #done.
There was a problem hiding this comment.
And if the $type isn't in the same namespace as the current method, the whole thing might become the discriminator. E.g. if you yield { $type: 'app.bsky.feed.post' } then the header would look like { op: 1, t: 'app.bsky.feed.post' }.
There was a problem hiding this comment.
yup that's right^^
altho this seemed strange since it was not in the schema 🤔
I'm down to add something back in tho
There was a problem hiding this comment.
Agree, the same case doesn't really happen now that codes is gone. I think it was a test of what happens when you yield a message with a type that doesn't have a matching code.
| $type: | ||
| i % 2 === 0 ? 'io.example.stream2#even' : 'io.example.stream2#odd', | ||
| $type: i % 2 === 0 ? '#even' : '#odd', |
There was a problem hiding this comment.
I think in a perfect world you can yield { $type: 'io.example.stream2#even' }, and #even get's placed as the discriminator in the header. I saw the compaction of the $type as being sort of an implementation detail of the wire protocol, and the application having the ability to deal with lex unions just as it normally would. Interested to hear your thinking on it!
There was a problem hiding this comment.
yup yup you're right. I fixed this up 👍
| const body = message.body | ||
| if (body && t !== undefined) { | ||
| body['$type'] = t.startsWith('#') ? this.opts.method + t : t | ||
| } |
There was a problem hiding this comment.
This mutates message— I'm pretty much good with it, just want to make sure it was intended. I suppose there could be a way to tag the message frame with the opts.method, and then make the the processed message.body a getter, or something like that.
There was a problem hiding this comment.
Ah, I guess it doesn't really matter since the message itself gets tossed!
There was a problem hiding this comment.
yeah probably not a huge worry but pushed up a change here so that we don't mutate body 👍
This integrates the lex changes from #698 into the subscribeRepos route
Namely work around removing InfoFrames & using the union ref on
messageto discriminate MessageFrames