aerogear / graphback

Graphback - Out of the box GraphQL server and client
https://graphback.dev
Apache License 2.0
409 stars 73 forks source link

Change plugin engine to operate on graphql compose schema #810

Closed wtrocki closed 4 years ago

wtrocki commented 4 years ago

Currently for creating schema we pass immutable schema object. We should operate on graphql compose schema and generate the immutable schema.after all plugins will finish. See core package for more details.

Interesting places: GraphbackMetadata - schema field (this in immutable) GraphbackPluginEngine - we passing this schema over Each plugin has some hacks.

The most interesting place to look on is OffixPlugin (to be Renamed to DataSync plugin)

wtrocki commented 4 years ago

This is the implementation that we need to use: https://graphql-compose.github.io/

wtrocki commented 4 years ago

@craicoverflow Can you add more information what is currently missing. On top of my head issues with the composer are:

Maybe we can investigate why this happens.

craicoverflow commented 4 years ago

schemaComposer.buildSchema() will throw an error if the data model does not have any root query defined.

This and the two issues you mentioned can be worked around (hacked to work) but this is far from ideal.

We should operate on graphql compose schema

Is this a good idea? We would be moving away from the reference implementation. As we saw in https://github.com/graphql-compose/graphql-compose/pull/237 there can be divergences which causes issues.

Obviously it seems like the best approach right now other than writing something ourself.

wtrocki commented 4 years ago

Considering how good and responsive GraphQL-Compose team is we might actually log issue for them to not remove comments etc. but we should make sure first if the problems are not on our side

wtrocki commented 4 years ago

I think that we good really good deal here already thanks to graphql compose contributor. Do you think we can do more? Maybe docs for plugin creation?

craicoverflow commented 4 years ago

I think that we good really good deal here already thanks to graphql compose contributor. Do you think we can do more? Maybe docs for plugin creation?

We could have a dummy plugin described in the docs that use graphql-compose or something, and recommend it. We don't want to push it too hard as the only way to do things.

wtrocki commented 4 years ago

I think offix plugin can become that example. Descriptions are back so things should be quite easy from now on. I might create some helpers in core but we will see how it goes once we start refining offix plugin

ssd71 commented 4 years ago

So we are going ahead with creating new plugin for this

wtrocki commented 4 years ago

@ssd71 For what specifically. There is PR that @cecobask done that was partially resolving the problem. https://github.com/aerogear/graphback/pull/829

ssd71 commented 4 years ago

I misunderstood, sorry about that! I will try some stuff and get back to you

craicoverflow commented 4 years ago

there was a fix done here https://github.com/aerogear/graphback/pull/912 - custom root fields were not getting added to the final schema.

This is a temporary solution, we believe that this current issue #810 will enable us to merge schemas more easily. So once this PR is complete @ssd71 we can work together to improve #912 :)

wtrocki commented 4 years ago

Some quick offline discussion with @craicoverflow and we decided to use graphql-compose across the board. This means that we can start work by rewriting SchemaPlugin to use graphql compose (currently does some hacks on graphql-js itself)

ssd71 commented 4 years ago

Understood, I will keep that in mind

ssd71 commented 4 years ago

From what @wtrocki said in #913 , the approach:

Have every plugin extending schema creating GraphQL compose instance making edits and then converting things back to normal schema.

Seems extremely sensible, I am working on this, will create PR in sometime

wtrocki commented 4 years ago

Looks like we have resolved all issues. Thx