-
Notifications
You must be signed in to change notification settings - Fork 6
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
support optional model api identifier in react hooks #201
Conversation
895dc40
to
842d75d
Compare
@@ -79,6 +79,37 @@ export const useAction = < | |||
transformedResult, | |||
useCallback( | |||
async (variables, context) => { | |||
if (action.hasAmbiguousIdentifier) { |
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.
This is the same logic that we generate in the API client
842d75d
to
dd395aa
Compare
packages/react/src/useAction.ts
Outdated
if (action.paramOnlyVariables?.includes(key)) { | ||
newVariables[key] = value; | ||
} else { | ||
if (key === "id") { |
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.
id
is kind of special in that if it's an update effect then we have to include it as a separate variable
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.
👍
Should we be checking if it's an update? Or can we look at action.variables
to see if it should be nested under the model API identifier or not? I'm just wondering if we could future-proof against creates where the end user can supply an id, but it won't be "lifted" to a variable like we do for updates.
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.
Changed this up to check action.variables
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.
Code LGTM, can you update the readme and docstrings of the hooks to make sure the docs reflect the new reality too?
c9eba9b
to
414eef5
Compare
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.
🚀
} else { | ||
newVariables = { | ||
[action.modelApiIdentifier]: {}, | ||
} as Exclude<F["variablesType"], null | undefined>; |
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.
This has to be casted? :(
Is it TypeScript not figuring it out, or do we have something typed incorrectly?
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 get Type '{ [x: string]: {}; }' is not assignable to type 'Exclude<F["variablesType"], null | undefined>'.
packages/react/src/useAction.ts
Outdated
if (action.paramOnlyVariables?.includes(key)) { | ||
newVariables[key] = value; | ||
} else { | ||
if (key === "id") { |
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.
👍
Should we be checking if it's an update? Or can we look at action.variables
to see if it should be nested under the model API identifier or not? I'm just wondering if we could future-proof against creates where the end user can supply an id, but it won't be "lifted" to a variable like we do for updates.
e440850
to
800ddcf
Compare
414eef5
to
715664a
Compare
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.
This works for me!
@@ -574,7 +570,7 @@ const [{ data, fetching, error }, _refetch] = useFindBy(api.blogPost.findBySlug, | |||
|
|||
### `useGlobalAction(actionFunction: GlobalActionFunction, options: UseGlobalActionOptions = {}): [{data, fetching, error}, refetch]` | |||
|
|||
`useGlobalAction` is a hook for running a backend Global Action. `useGlobalAction(api.widget.create)` is the React equivalent of `await api.someGlobalAction({...})`. `useGlobalAction` doesn't immediately dispatch a request to run an action server side, but instead returns a result object and a function which runs the action, similar to [`urql`'s `useMutation` hook](https://formidable.com/open-source/urql/docs/api/urql/#usemutation). `useGlobalAction` must be passed one of the global action functions from an instance of your application's generated API client. Options: | |||
`useGlobalAction` is a hook for running a backend Global Action. `useGlobalAction(api.someGlobalAction)` is the React equivalent of `await api.someGlobalAction({...})`. `useGlobalAction` doesn't immediately dispatch a request to run an action server side, but instead returns a result object and a function which runs the action, similar to [`urql`'s `useMutation` hook](https://formidable.com/open-source/urql/docs/api/urql/#usemutation). `useGlobalAction` must be passed one of the global action functions from an instance of your application's generated API client. Options: |
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 need to update the react hooks to also allow optional model api identifiers. This follows the same logic that the internal and public model managers.
closes GGT-4093
PR Checklist