Skip to content
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

Open
MrLoh opened this issue Oct 28, 2017 · 16 comments
Open

enable type casting #9

MrLoh opened this issue Oct 28, 2017 · 16 comments

Comments

@MrLoh
Copy link

MrLoh commented Oct 28, 2017

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
Copy link
Author

MrLoh commented Oct 28, 2017

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
Copy link
Author

MrLoh commented Oct 28, 2017

maybe it should better look like this:

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

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Nov 1, 2017

@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
Copy link
Author

MrLoh commented Nov 1, 2017

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
Copy link
Author

MrLoh commented Nov 1, 2017

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
Copy link
Author

MrLoh commented Nov 1, 2017

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

@MrLoh
Copy link
Author

MrLoh commented Nov 1, 2017

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.
apollographql/apollo-client#585 (comment)

@MrLoh
Copy link
Author

MrLoh commented Nov 1, 2017

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

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Nov 10, 2017

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
Copy link
Author

MrLoh commented Nov 10, 2017

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
Copy link
Author

MrLoh commented Nov 10, 2017

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
Copy link
Member

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.

@MrLoh
Copy link
Author

MrLoh commented Nov 10, 2017

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
Copy link
Author

MrLoh commented Nov 10, 2017

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
Copy link
Member

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

@hutber
Copy link

hutber commented Aug 16, 2022

Blimey, its been 5 years almost since this!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants