Skip to content

implement permission-set types#84

Merged
tylersayshi merged 11 commits intotylersayshi:mainfrom
jphastings:permission-sets
Apr 6, 2026
Merged

implement permission-set types#84
tylersayshi merged 11 commits intotylersayshi:mainfrom
jphastings:permission-sets

Conversation

@jphastings
Copy link
Copy Markdown
Contributor

@jphastings jphastings commented Apr 3, 2026

Hey @tylersayshi I had a crack at implementing #82!

I kept everything in lib.ts to fit with your current layout, but it's starting to get a bit large, so let me know if you'd like things broken out.

I've also added a helper function for fields which have companion *:lang fields, so translations can be grouped with their default!

{
  title: lx.langString("The default", {
    "es": "El predeterminado",
    "ja": "デフォルト",
  }),
}

Becomes:

{
  "title": "The default",
  "title:lang": {
    "es": "El predeterminado",
    "ja": "デフォルト",
  }
}

Fixes #82

Adds support for permission-set lexicons with `lx.permissionSet()` and permission entry builders:

- `lx.repoPermission()`
- `lx.rpcPermission()`
- `lx.blobPermission()`
- `lx.accountPermission()`
- `lx.identityPermission()`

Collections and endpoints accept both lexicon schema objects and plain NSID strings.
Allows easy grouping of translations with their default, for clarity.
Copy link
Copy Markdown
Owner

@tylersayshi tylersayshi left a comment

Choose a reason for hiding this comment

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

overall looks good! thanks for taking this on 😺

and I'm not worried about lib.ts getting large - I don't expect it to grow much further and am in general in favor of having that single large file with the implementation.

@jphastings
Copy link
Copy Markdown
Contributor Author

Thank you for the detailed review! I thoroughly agree — so much of the benefit of your package is great document in-IDE! I'll make the improvements and update shortly

@jphastings
Copy link
Copy Markdown
Contributor Author

Hey @tylersayshi; I've addressed the points you made, though I'm struggling with maintaining both the key ordering in the snaps as you requested and ensuring the tests pass. If you have any advice I'm all ears!

@tylersayshi
Copy link
Copy Markdown
Owner

hey! sorry there was a setting blocking the CI from running (these should run automatically going forward, i changeed the setting). Pretty much everything looks good now, just some tests that need to get fixed.

One last thing i'd like this PR to add to is the benchmark test: https://github.com/tylersayshi/prototypey/blob/main/packages/prototypey/core/tests/infer.bench.ts

it's ok (and expected) if more instantiations are caused by this change, but if the number goes up significantly, i'll like to look into the type inference implementation closer.

@jphastings
Copy link
Copy Markdown
Contributor Author

Sure thing @tylersayshi — the test failures are just the ordering issues, so I'll fix those now & work on the benchmarks.

@jphastings
Copy link
Copy Markdown
Contributor Author

So I've added some benchmarks, and run the existing ones — I think this is only adding a small increase in instantiations. I'm not familiar with this analysis in Typescript, so I had to ask Claude to help out here, but here are it's results:

Benchmark Baseline (main) Branch Δ
infer with simple object 685 718 +33 (+4.8%)
infer with complex nested structure 956 1011 +55 (+5.8%)
infer with circular reference 634 685 +51 (+8.0%)
infer with app.bsky.feed.defs 1237 1327 +90 (+7.3%)
fromJSON infer with simple object 438 463 +25 (+5.7%)
fromJSON infer with complex 499 524 +25 (+5.0%)
fromJSON infer with circular 411 436 +25 (+6.1%)
fromJSON infer with app.bsky.feed.defs 513 538 +25 (+4.9%)

What do you think?

Copy link
Copy Markdown
Owner

@tylersayshi tylersayshi left a comment

Choose a reason for hiding this comment

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

yep, the benchmarks look good 👍 thanks for building this!

@tylersayshi tylersayshi merged commit eaedcb7 into tylersayshi:main Apr 6, 2026
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.

Support for permission-set lexicons?

2 participants