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

lex: update bsky schemas to have actual record refs, not 'unknown' #2860

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bnewbold
Copy link
Collaborator

@bnewbold bnewbold commented Oct 4, 2024

This certainly makes more sense to me for things like the "view" types, where we definitely know the type that is going to appear.

As a general style things, feels like we could use an "empty open union" in more cases where we are indicating a record?

@mary-ext
Copy link
Contributor

mary-ext commented Oct 4, 2024

I believe the TypeScript lex-cli is currently not set up around refs linking to records, app.bsky.feed.post would be linked as AppBskyFeedPost.Main (which doesn't exist), rather than AppBskyFeedPost.Record

we don't check the refs when generating them (so it's also possible to reference nonexistent defs)

export function refToType(
ref: string,
baseNsid: string,
imports: Set<string>,
): string {
// TODO: import external types!
let [refBase, refHash] = ref.split('#')
refBase = stripScheme(refBase)
if (!refHash) refHash = 'main'
// internal
if (!refBase || baseNsid === refBase) {
return toTitleCase(refHash)
}
// external
imports.add(refBase)
return `${toTitleCase(refBase)}.${toTitleCase(refHash)}`
}

@bnewbold
Copy link
Collaborator Author

bnewbold commented Oct 4, 2024

Talked to @pfrazee some more about this, and to some degree it is about keeping the schemas flexible. We could in theory end up with a "app.bsky.feed.postV2" (or "app.bsky.feed.post.v2" or whatever) iterated record type, and might want to pass that back in the existing view APIs. We could also iterate on the view API overall. My take is that this would be better served with either an open union (instead of "unknown"), and probably add a "view v2" endpoint which supports records of either type. AKA, make the future view APIs backwards compatible, instead of trying to make current view API forwards-compatible. I suspect plenty of folks have implemented clients in ways such that a different record type showing up in a "post view" would result in an error.

@bnewbold bnewbold requested review from devinivy and dholms October 4, 2024 21:19
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