graphql-lodash icon indicating copy to clipboard operation
graphql-lodash copied to clipboard

enable type casting

Open MrLoh opened this issue 8 years ago • 16 comments

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 avatar Oct 28 '17 14:10 MrLoh

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 avatar Oct 28 '17 14:10 MrLoh

maybe it should better look like this:

movie {
  title
  releaseDate @_(asDate: none)
}

MrLoh avatar Oct 28 '17 14:10 MrLoh

@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)?

IvanGoncharov avatar Nov 01 '17 15:11 IvanGoncharov

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 avatar Nov 01 '17 16:11 MrLoh

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 avatar Nov 01 '17 16:11 MrLoh

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 avatar Nov 01 '17 16:11 MrLoh

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 avatar Nov 01 '17 16:11 MrLoh

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

MrLoh avatar Nov 01 '17 17:11 MrLoh

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?

IvanGoncharov avatar Nov 10 '17 10:11 IvanGoncharov

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 avatar Nov 10 '17 10:11 MrLoh

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

MrLoh avatar Nov 10 '17 10:11 MrLoh

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:

  • Currently I just expose functions from lodash so I don't need to document them because they documented in lodash docs.
  • If I map all lodash functions than autocompletion list will be already huge, but if I add, even more, transformations it will make autocompletion unusable.

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.

IvanGoncharov avatar Nov 10 '17 11:11 IvanGoncharov

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 avatar Nov 10 '17 11:11 MrLoh

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.

MrLoh avatar Nov 10 '17 16:11 MrLoh

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

IvanGoncharov avatar Nov 10 '17 17:11 IvanGoncharov

Blimey, its been 5 years almost since this!!

hutber avatar Aug 16 '22 07:08 hutber