-
Notifications
You must be signed in to change notification settings - Fork 48
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
enable type casting #9
Comments
I tried to build something into the direction myself, but I can't get it to work. Maybe I'm missing some understanding for how the functions are called. |
maybe it should better look like this:
|
@MrLoh I looked into your implementation and left a comment on how to make it work. I think for integers and strings it's better to use Before adding it I want to better understand your use case. |
I'm sending Dates over graphql and since Date objects are not directly supported, I need to parse them in the query, which in the prop transformations in Apollo, which is a bit of a pain, because you get code like this, only to convert the date. graphql(MovieQuery, {
props: ({ data: { movie }, ...props }) => ({
...props,
movie: movie && {
...movie,
showtimes: showtimes && showtimes.map(({datetime, ...showtime}) => ({
...showtime,
datetime: datetime && new Date(datetime)
}))
}
)}
}) |
In another part of the API, I'm getting geo coordinates back as Strings, but need them as Numbers, which leads to similar code. |
But I think dates are the most common use case, since you can't send them natively and apollo doesn't provide an easy way to decode them apollographql/apollo-client#585 |
I also found this proposal super interesting, but I'm not even sure how something like that could be implemented. I found your approach to be closest to that. |
I didn't know the lodash functions |
@MrLoh Not at the moment but it pretty easy to add them. It will require the only couple of lines per function. PR is welcome 😉
Thanks for details about your use case, now I understand your pain point. let { query, transform } = graphqlLodash(queryWithLodash, {
plugins: [
require('graphql-lodash-toDate'),
{
sdl: `directive @toRegExp(flags: String) on FIELD`,
transformation: (value, args) => new RegExp(value, args.flags)
}
]
}); It will allow both extending beyond lodash transformation and project-specific transformations. |
Yeah I was also thinking about that. I think allowing to add custom transformations would be great. Maybe without the need to create a package though, just being able to pass an array or object of functions would be ideal I think. |
Maybe you still want to consider adding a toDate function anyways, so that toNunber and toString have an equivalent. I think parsing dates is one of the most common transformations people need in graphql |
Idea is to support both since packages will just export object: const toDate = {
sdl: `directive @toDate on FIELD`,
transformation: (value) => new Date(value)
}
export default toDate;
There is couple problem with adding
Most important one: Currently this lib accepts JSON and outputs JSON so you can do a transformation on a server if you want. Here is cool example where server transformation useful. You can't send Also how it should be displayed in GraphiQL? At the same time, I understand your use case and don't want to limit other developers who want to use it with Apollo. I think adding one more npm package without any additional dependencies is not a big price to pay so it would be ideal if If something makes me reconsider that decision it always possible to add it to the core of |
Yeah, that makes a lot of sense totally convinced that this is not a good idea to include it for the reasons you mention. Don't overload the library with too much functionality, that can lead to confusion. I think allowing custom extensions would hit the sweet spot, as it keeps the library clean and surface area for bugs small and gives developers the freedom to extend it with what they need. |
Is there any way I could help to move this forward. It seems like you already have a rough idea, how the API could look like. Let me know if there is a way to help. I'll definitely be happy to test anything. |
I will try to find some time on this weekend to release |
Blimey, its been 5 years almost since this!! |
One of the very common transformations I need (except from the ones that this library already enables) is casting a value into a Date. Would it be possible to add a function for that like:
The text was updated successfully, but these errors were encountered: