search

deInternetJongens / Lighthouse-Utils

An add-on to Lighthouse to auto-generate CRUD actions from types https://github.com/nuwave/lighthouse
MIT License
26 stars 2 forks source link

Outsource scalars to seperate package #13

Open spawnia opened 5 years ago

spawnia commented 5 years ago

Hi, i started a package over at https://github.com/mll-lab/graphql-php-scalars to gather custom scalars.

I see you implemented a few of your own. It would be nice to concentrate the efforts towards a shared, well-tested collection. Let me know what you think.

jordyvandomselaar commented 5 years ago

Hey Benedikt,

The package you started looks great! I think it'd be a great idea to combine our efforts where possible.

@maarten00 what do you think?

maarten00 commented 5 years ago

Sounds like a great idea! Feel free to use the scalars we've created. I don't think we'll need to integrate this package with your scalars package, as this package on it's own doesn't really need scalars. So I'll probably remove them later on.

Our Date and DateTimeTZ scalars are a bit opinionated. We expect devs to implement the casting in the Eloquent models. Not sure what your take would be on that, but I'm open to changing that if you want.

We have a Postalcode scalar for Dutch postal codes, but maybe we could scrap that and use your regex scalar type. Or we could extend it from the regex type and provide regex presets for different countries and allow passing a custom regex.

spawnia commented 5 years ago

Nice! I will probably add the scalars package as a dependency for Lighthouse anyways, once i have it all fleshed out.

There are some pretty tricky things in there that are hard to get exactly right, as well as a lot of boilerplate when it comes to types that are in essence a subset of String, Int, whatever.

Lighthouse, as well as your package, might still need some more opinionated types - for example, all date related Scalars should be instanceof Carbon in Laravel, but not necessarily in other PHP frameworks. I think there is still value in having a bunch of utils that simplify the most common tasks when writing custom scalars.

maarten00 commented 5 years ago

In that case it would probably be a good idea to move the parsing of a string to a Carbon object to the scalar. Sounds like a good idea to me.