graphql-kit / graphql-lodash

🛠 Data manipulation for GraphQL queries with lodash syntax
https://apis.guru/graphql-lodash/
MIT License
1.23k stars 50 forks source link

enable type casting #9

Open MrLoh opened 6 years ago

MrLoh commented 6 years ago

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:

movie {
  title
  releaseDate @_(as: "Date")
}
MrLoh commented 6 years ago

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.

https://github.com/MrLoh/graphql-lodash/blob/ddeff5e4dccd91f4e4c0b15c95a71c12824d7d19/src/transformations.ts#L43

MrLoh commented 6 years ago

maybe it should better look like this:

movie {
  title
  releaseDate @_(asDate: none)
}
IvanGoncharov commented 6 years ago

@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 toString, toInteger, toNumber. So if we decide to add Date transformation it also should be toDate.

Before adding it I want to better understand your use case. Do you try to convert integer with Unix time into a string with a date? Or you want to do new Date(stringFromGraphQL)?

MrLoh commented 6 years ago

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)
            }))
        }
    )}
})
MrLoh commented 6 years ago

In another part of the API, I'm getting geo coordinates back as Strings, but need them as Numbers, which leads to similar code.

MrLoh commented 6 years ago

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 https://github.com/apollographql/apollo-client/issues/585

MrLoh commented 6 years ago

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. https://github.com/apollographql/apollo-client/issues/585#issuecomment-272625302

MrLoh commented 6 years ago

I didn't know the lodash functions toNumber, etc. and they are not supported yet by this lib, either, are they?

IvanGoncharov commented 6 years ago

I didn't know the lodash functions toNumber, etc. and they are not supported yet by this lib, either, are they?

@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 😉

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.

Thanks for details about your use case, now I understand your pain point. This makes me think what about adding the ability to add plugin directives, like @spread or @toDate they can leave as separate packages like graphql-lodash-toDate and you would plug it like that:

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. What do you think?

MrLoh commented 6 years ago

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.

MrLoh commented 6 years ago

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

IvanGoncharov commented 6 years ago

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.

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;

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

There is couple problem with adding toDate:

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 new Date(...) from server to client so @toDate will be useless and confusing for the case when the transformation is applied on a server.

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 @toDate will live in a separate package.

If something makes me reconsider that decision it always possible to add it to the core of graphql-lodash.

MrLoh commented 6 years ago

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.

MrLoh commented 6 years ago

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.

IvanGoncharov commented 6 years ago

I will try to find some time on this weekend to release alpha version. So you could test it by implementing toDate.

hutber commented 1 year ago

Blimey, its been 5 years almost since this!!