-
Notifications
You must be signed in to change notification settings - Fork 563
fix(postgrest): preserve nullability when using JSON path #1939
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
fix(postgrest): preserve nullability when using JSON path #1939
Conversation
…upabase#1635) The JSON path resolver (JsonPathToType) stripped `null` using Exclude<T, null> but never added it back to the resulting type. This caused `.select('col->a')` to infer a non-nullable type even when the underlying JSON column was nullable. Example: json_col: { a: string } | null .select('json_col->a') // inferred `string` instead of `string | null` This patch re-attaches `| null` when the root JSON column contains null, ensuring correct type inference for nullable JSON columns and nullable nested paths. Fixes: supabase#1635
60a5dcc to
621e051
Compare
|
Thanks for the fix @dev-hari-prasad ! This correctly addresses the nullability issue in #1635. However, since this changes type inference in a way that could cause compilation errors for existing users (nullable JSON paths will now correctly return Before merging, could you please:
I'll tag this for v3 (next major release). And we can merge it when the time comes! |
Sure |
Added tests to verify JsonPathToType correctly returns T | null when accessing paths on nullable JSON columns. Also updated existing tests to reflect the new nullable behavior in type expectations.
|
@mandarini Tests are now added, let me know if anything else is required |
|
Thanks @dev-hari-prasad ! We'll wait for v3 now! |
avallete
left a comment
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.
A few comments, I think we should add more tests over the runtime behavior of this to ensure we don't regress on it further down the line.
|
Happy to update or rewrite the affected tests as part of this pr. Just let me know if you’d like me to take that on, or if you prefer to handle it internally. |
Hey @dev-hari-prasad first of all, thanks a lot for the contribution! You’re very welcome to address the comments I left, that would be much appreciated and should help speed up getting this merged and released. Otherwise, I can look into adding or updating the tests myself once I have the bandwidth. |
…l-should-be-included-in-return-type
|
I don't expect this to really be a BC anymore. But as @mandarini mentioned, some users might rely over the "non null" behavior so that might break their typecheck (even if that's more correct). |
|
hmmm i don't know what happened, i did not close this. give me a moment |
|
i merged the other PR so base branch got deleted. give me a moment to fix this |
|
Yeah, something weird happen, and we cannot reopen the PR and target the master branch, this is what github copilot says: What happened to this PRThis PR was automatically closed when its base branch ( Timeline of events:
Why GitHub closed it instead of retargeting:The head branch ( Rather than auto-retarget and show duplicate commits, GitHub closed the PR. Next steps:I created a new PR (#2011) that:
Thanks @dev-hari-prasad for the contribution! The work continues in the new PR. |
Description:
What changed?
Fixed the type inference for JSON path selections (
->) by adding| nullwhen the underlying JSON column can be null.Why was this change needed?
When selecting a JSON path from a nullable JSON column, the generated TypeScript types incorrectly assumed the value could never be null.
In reality, the runtime returns
nullwhen the whole column is null, so the types were unsafe.This patch brings the inferred type in line with actual behavior.
Closes/Fixes #1635
Screenshots/Examples:
N/A since it is a type-only change.
Breaking changes:
This PR contains no breaking changes.
📋 Checklist:
<type>(<scope>): <description>npx nx formatto ensure consistent code formattingAdditional notes:
The change is intentionally minimal and only affects the nullable branch of
JsonPathToType.It preserves existing behavior for non-nullable JSON columns while correctly reflecting actual return values for nullable ones.
CC @avallete & @mandarini