Skip to content

Remove non-spec boolean required and nullable keys from properties#85

Open
jphastings wants to merge 3 commits intotylersayshi:mainfrom
jphastings:fix-unnecessary-required-attribute
Open

Remove non-spec boolean required and nullable keys from properties#85
jphastings wants to merge 3 commits intotylersayshi:mainfrom
jphastings:fix-unnecessary-required-attribute

Conversation

@jphastings
Copy link
Copy Markdown
Contributor

@jphastings jphastings commented Apr 6, 2026

I noticed that required: true and nullable: true were being left on the JSON lexicon definitions as well as being hoisted to the required: […], nullable: […] arrays.

I did some research, and while they aren't explicitly excluded in the spec, there's no indication that they should be present that I can find.

I'm almost convinced they shouldn't be there, as the bsky lexicons don't have them,, they're both canonically defined as being the arrays prototypey already hoists to (as per the spec), and JSON Schema (on which the lexicon spec is based) doesn't include these fields in this way either.

I think these were accidentally included because the code is a lot neater if they're left in 😅

I've updated the tests to exclude these fields (first commit), and created a function to do the extraction (second commit).

Do let me know if you have any questions!

They aren't *explicitly* excluded in the spec, but there's no indication that they *should* be present, the bsky lexicons don't have them (https://lexicon.garden/lexicon/did:plc:4v4y5r3lwsbtmsxhile2ljac/app.bsky.feed.searchPosts), `required` and `nullable` are called out as being canonically defined elsewhere (https://atproto.com/specs/lexicon#object), and JSON Schema (on which the lexicon spec is based) doesn't include these fields in this way either.

I think these were accidentally included because the code can be a lot neater if they're left in!
This commit resolves the issues illustrated by the previous commit. Now the required and nullable values are hoisted to the parent, but are also removed from inside the properties themselves.

Has special handling for nested objects, where the `require` property pulls double duty (as prototypey's boolean indicator of the parent, and as the lexicon's array on the child)
@jphastings jphastings force-pushed the fix-unnecessary-required-attribute branch from c824f37 to dfa1d06 Compare April 6, 2026 21:32
@jphastings
Copy link
Copy Markdown
Contributor Author

This is good for review now too @tylersayshi! Let me know if you have any concerns

@tylersayshi
Copy link
Copy Markdown
Owner

I think these were accidentally included because the code is a lot neater if they're left in 😅

this is essentially right. I left them in because the library can stay simpler in that case. It was intentional though.

I'm open to changing this position, but I think there'd need to be a reason to remove the fields beyond just that they're non-functional in being there.

If anything, I'd lean towards stripping these in the genJson step to keep the core library simpler.

Please push back on any or all of this, but this is where I'm at for now.

@jphastings
Copy link
Copy Markdown
Contributor Author

jphastings commented Apr 7, 2026

Hey Tyler, I definitely believe we should remove the required: trues from the JSON output; round-tripping a 3rd-party lexicon should result in equal output. (Especially for the next piece I'll try to tackle, working with 3rd party lexicons!)

Additionally, as I was trying to make a demonstration of the issue, I realized that I there's no way to specify that an object is required when using prototypey's syntax: lx.object({ propA: lx.string({ required: true }) }).

I think you intended to make ObjectOptions include required and nullable values, given how the @see link points to the docs declaring those fields, but both of those fields are absent at the moment.

I'm going to keep exploring to see if I can get that working, which I think would be a good addition to this PR, to get required and nullable showing only in their spec-compliant places in all forms!

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