-
Notifications
You must be signed in to change notification settings - Fork 511
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
Improve codegen bundle size and typings #2999
base: main
Are you sure you want to change the base?
Conversation
9ed3816
to
5400766
Compare
2abd90e
to
ef8af3f
Compare
bebca0e
to
0bea135
Compare
0bea135
to
02ddb9a
Compare
02ddb9a
to
1a337a2
Compare
cd packages/pds; pnpm run codegen | ||
cd packages/bsky; pnpm run codegen | ||
cd packages/ozone; pnpm run codegen | ||
pnpm run --parallel codegen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, we could do this more similarly to how we do the build
task. That is, do a pnpm codegen
here on the makefile, and "codegen": "pnpm run --parallel codegen",
in package.json
.
@@ -1,24 +1,18 @@ | |||
const TID_LENGTH = 13 | |||
const TID_REGEX = /^[234567abcdefghij][234567abcdefghijklmnopqrstuvwxyz]{12}$/ | |||
|
|||
export const ensureValidTid = (tid: string): void => { | |||
if (tid.length !== 13) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to use the constant here.
export function isObj(obj: unknown): obj is Record<string, unknown> { | ||
return obj !== null && typeof obj === 'object' | ||
export function isObj<V>(v: V): v is V & object { | ||
return v != null && typeof v === 'object' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found it a bit weird that we would want to use !=
instead of !==
. In this case, it doesn't seem to make a difference. Just mentioning in case you did it by accident. Could probably be !!v
as well, so it is less ambiguous.
BTW, are we fine with isObj
returning true
for arrays? I'm just wondering if that's the behavior we expect in this project from isObj
, despite arrays being objects at the language level.
This PR is the first of a 3 step changes aimed at improving typing in the atproto repo:
$type
property to records and user object typesNote
When reviewing, "codegen" commits can be ignored as they only contain generated code changes.