-
-
Notifications
You must be signed in to change notification settings - Fork 287
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
Rest datasource configure bodies and headers #721
Conversation
Your Render PR Server URL is https://toolpad-pr-721.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cbkev84gqg48i38bisdg. |
Your Render PR Server URL is https://toolpad-pr-721.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cbqag1pa6gdkkqls4isg. |
Your Render PR Server URL is https://toolpad-pr-721.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cc8ssula499fvmbkjr8g. |
cool!
|
@@ -201,6 +211,27 @@ function QueryEditor({ | |||
})); | |||
}, []); | |||
|
|||
const handleBodyChange = React.useCallback((newBody: Maybe<Body>) => { |
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 have a reusable function for these? the callbacks seem pretty similar.
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.
but if we predict these might change over time it's best to keep them separate though, just a thought
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.
One could invent a hook like:
function useQueryChangeHandler<F extends Function>(
setInput: React.Dispatch<React.SetStateAction<QueryEditorModel<FetchQuery>>>,
property: string,
producer: F,
): F {
return React.useCallback<F>(
(...args: any[]) => {
setInput((existing) => ({
...existing,
query: { ...existing.query, [property]: producer(...args) },
}));
},
[producer, property, setInput],
);
}
and use as
const handleMethodChange = useQueryChangeHandler(setInput, 'method', (event) => event.target.value);
const handleMethodChange = useQueryChangeHandler(setInput, 'url', (newUrl) => newUrl);
// ...
But that produce
parameter would invalidate the callback on each render. So we could declare those functions in module scope, or wrap them with useCallback
as well, but at which point do all the abstractions become less readable again? The way it's done now is idiomatic and explicit and easy to understand. I kind of prefer it, even though it could probably be expressed in less lines of code. I was thinking for the future we could use immer
to remove some of the duplication inside the setInput
.
e.g. from
const handleMethodChange = React.useCallback((event: React.ChangeEvent<HTMLInputElement>) => {
setInput((existing) => ({
...existing,
query: { ...existing.query, method: event.target.value },
}));
}, []);
to
const handleMethodChange = React.useCallback(
(event: React.ChangeEvent<HTMLInputElement>) =>
setInput(
produce((draft) => {
draft.query.method = event.target.value;
}),
),
[],
);
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.
yeah, it's probably not worth introducing something so complex yet at least (we could not use a producer
function/argument, but then we would probably be making the callback less flexible)
it was just a thought, i think we can keep this as it is it's pretty easy to read and understand.
i agree with using immer
later for this kind of operations though, makes them a bit cleaner!
It's parameters to the datasource query itself. this is where you can bind state of the page, which is usable inside of the query bindings. I moved it to this place because it's the same location as in the function and postgresql datasources. |
ah i see, makes sense to have them outside the tabs then |
@@ -40,7 +40,7 @@ import { | |||
NodeRuntimeWrapper, | |||
ResetNodeErrorsKeyProvider, | |||
} from '@mui/toolpad-core/runtime'; | |||
import { pick } from 'lodash-es'; | |||
import * as _ from 'lodash-es'; |
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.
minor thing - just wondering if we are using something to treeshake such imports?
In the past I've used babel-transform-imports maybe in the future it would be worth to look into that 🤔
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.
It is tree shaken by the swc
minifier in next.js. The import style doesn't influence the behavior.
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.
Oh nice, thanks, I didn't know that 👍
Screen.Recording.2022-09-05.at.12.29.15.mov
Fixes #850