-
Notifications
You must be signed in to change notification settings - Fork 1
fix(core): change ValidProjection to string type #627
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(core): change ValidProjection to string type #627
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Pull Request Overview
This PR changes the ValidProjection type from a strict template literal type {${string}} to a more flexible string type to reduce user confusion and type errors. The change deprecates the original type while maintaining backward compatibility.
Key changes:
- Deprecates the strict template literal
ValidProjectiontype and changes it tostring - Updates all function signatures to use
stringinstead ofValidProjectionfor projection parameters - Adds runtime validation through
validateProjectionfunction calls
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/hooks/projection/useDocumentProjection.ts | Updates hook generic constraints from ValidProjection to string |
| packages/react/src/hooks/projection/useDocumentProjection.test.tsx | Updates test component type signatures to use string instead of ValidProjection |
| packages/core/src/projection/util.ts | Changes validateProjection return type from ValidProjection to string |
| packages/core/src/projection/types.ts | Deprecates ValidProjection type and redefines it as string |
| packages/core/src/projection/resolveProjection.ts | Updates function generic constraint from ValidProjection to string |
| packages/core/src/projection/resolveProjection.test.ts | Updates test to use plain string instead of ValidProjection type assertion |
| packages/core/src/projection/projectionQuery.ts | Adds runtime validation call and updates type references |
| packages/core/src/projection/projectionQuery.test.ts | Updates test variable types from ValidProjection to string |
| packages/core/src/projection/getProjectionState.ts | Updates function generic constraints from ValidProjection to string |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
8d4b6d6 to
d4695ae
Compare
|
|
||
| /** | ||
| * @public | ||
| * @deprecated |
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.
(question) do we want to mark this as deprecated or do we want to mark the version that this goes out with as a breaking change?
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.
We can certainly use it as a reason to go up a version but I can't think of a scenario in which this might break someone's builds. The type has become more permissive so anyone who used ValidProjection explicitly shouldn't get any type errors, I don't think (maybe some linting against deprecations?)...
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.
Good point, it likely would never cause any issues for users. I think I would rather remove it than mark it as deprecated in this case. What do you think?
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 think removing an export would be a breaking change and require going up a version. We can do that too but maybe want to strategize on breaking changes first.
It would be good to add a todo list item to remove all deprecated types when we go up a version, certainly.
Description
The template literal type for ValidProjection was a bit too strict, pretty much always resulting in error and user confusion. As far as I can tell there is no way to declare this type to be a bit more forgiving without making something kind of messy, so I've deprecated it and changed it to a string.
I've kept the type for backwards compatibility but redeclared it to a string as well.
What to review
Very little has changed -- I just wanted to ensure that any function that received a projection as a parameter would validate the projection.
Testing
Existing tests should suffice.
Fun gif