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

Request for clarification about project's goals #383

Closed
nagledb opened this issue Nov 1, 2018 · 3 comments
Closed

Request for clarification about project's goals #383

nagledb opened this issue Nov 1, 2018 · 3 comments

Comments

@nagledb
Copy link
Contributor

nagledb commented Nov 1, 2018

In Feature Request: Directive Visitors / Resolvers, @vladar states:

We follow graphql-js reference implementation which doesn't provide such tools at the moment.

Over at the nuwave/lighthouse project, in New GraphQL implementation, @vladar states:

I am not against competition even though I think it would be much better for the PHP GraphQL community to have a single base library to avoid community fragmentation.

But since you guys never tried to reach me out to discuss this, competition is fine too %) But I'd like it to be fair. So please when you criticise other people's work - at least give some links so that we could improve. I am talking about this part:

I've heard that people have had trouble implementing features like Schema Decorators on top of webonyx/graphql-php

In the first, you say you want to follow the graphql-js reference implementation and appear to cite that as a reason for why you don't want to add additional features. But then in the second, you say you want this to be the single graphql library the community forms around and suggest that you're open to areas for improvement. The thing that really stands out to me is that the features in question are very similar between the two threads: one is discussing directives including schema directives, whereas the other references schema decorators which are functionally very similar to schema directives.

I think both goals are reasonable. It's very reasonable to focus on following the reference implementation. It's also very reasonable to want the community to invest in a single GraphQL implementation and expand it to meet people's needs. But those two goals are fundamentally in conflict. I don't think you can do both effectively. (I think Apollo is pretty good proof that the reference implementation doesn't adequately meet people's needs.)

Could you please clarify which of the two goals is going to be your primary focus going forward?

I apologize if it seems like a pushy question, but it's an important decision-making point for me. If you intend to continue adhering strictly to the reference implementation, then that tells me I shouldn't bother making feature requests (or pull requests) that are inconsistent with the reference implementation. It also tells me that maybe I should seriously consider another library if it provides features I need that I feel this library is lacking (case in point, easy support for custom directives). And given the discussion in that thread on the Lighthouse project, I doubt I'm the only one who would find the clarification valuable.

@vladar
Copy link
Member

vladar commented Nov 1, 2018

As for Feature Request: Directive Visitors / Resolvers - it basically asks to port schemaVisitor from graphql-tools. But it doesn't require changes in this library core. Anyone can implement it on top of graphql-php library similar to how graphql-tools utilize graphql-js.

We just don't have resources to maintain all the features and 3rd-party projects inside this core library. But someone else can do it in a separate project (same as apollo does in their separate projects - they just wrap graphql-js)!

The thing is that if you have single core library - you get single project like graphql-tools built on top of it. But if you have multiple core libraries - each of them will require such higher level tools implemented independently. In reality you won't get 3 graphql-tools-php, you will probably get one for one core library, then you will have other useful higher-level library written for the other core-library. Hence fragmentation.

I think both goals are reasonable. It's very reasonable to focus on following the reference implementation. It's also very reasonable to want the community to invest in a single GraphQL implementation and expand it to meet people's needs.

Not expand it, but build on top of it the same way Apollo builds on top of graphql-js. The core library should provide spec-compatible means to extend it.

But those two goals are fundamentally in conflict.

Why so? I don't see any arguments in favor of this claim.

(I think Apollo is pretty good proof that the reference implementation doesn't adequately meet people's needs.)

Apollo uses reference implementation under the hood basically everywhere. They don't replace it. They build on top of it.

One problem with the reference implementation (which we probably inherited) is that they include too much in their core. Things like buildSchema or findBreakingChanges are useful but they are not a part of the specification. So they could have put them in some other repository instead. Apollo do replace some of these tools (at least buildSchema), but that just prooves that the core includes too much now.

Could you please clarify which of the two goals is going to be your primary focus going forward?

Sine I don't see a fundamental conflict in those two goals, I can't give the answer you would like.

Our goal is to be fully spec-compliant but at the same time be open for spec-compliant extensions. The easiest way to be spec-compliant is to follow the reference implementation. But if reference implementation doesn't allow some spec-compliant extensions we are open to deviate from it (as we did with buildSchema by allowing $typeConfigDecorator argument which doesn't exist in the reference implementation).

But the truth is that the most important features eventually show up in the reference implementation itself! We had multiple sitations like this (in early stages of the library life) when we implemented some feature which was missing in the reference implementation only to find out that they added it later in a slightly different way and we had to introduce a BC break.

It also tells me that maybe I should seriously consider another library if it provides features I need that I feel this library is lacking (case in point, easy support for custom directives).

Once again - if you are talking about #299 - you can implement it yourself on top of this library same way as apollo implemented it on top of graphql-js. If you won't be able to do so because of some of the library limitations we are ready to introduce API changes required for you to go forward.

Even with this example - Apollo team didn't open PR in the reference implementation asking to include this feature, because it is unnecessary.

@vladar
Copy link
Member

vladar commented Nov 1, 2018

The thing that really stands out to me is that the features in question are very similar between the two threads: one is discussing directives including schema directives, whereas the other references schema decorators which are functionally very similar to schema directives.

First of all, schema directives are already supported in the core. It's some convenience tools that are missing and which were proposed as a feature request in #299 (and they can be implemented on top of this library by anyone as I already mentioned).

Schema decorators, on the other hand, is the spec-incompatible proposal. They are indeed hard to implement in the webonyx core because they break the GraphQL specification. Technically it was an experimental fork of the GraphQL itself (which was later proposed as a PR in graphql-js, which later turned into schema directives).

There is little benefit allowing spec-incompatible extensions points in the library core (like customizable parsers). You can still do it by forking the library and then proposing your experimental features to GraphQL spec (which would later show up in the reference implementation if accepted).

@nagledb
Copy link
Contributor Author

nagledb commented Nov 1, 2018

Thank you for the thoughtful and patient response, @vladar!

I actually didn't realize that Apollo was built on top of the reference implementation. That misunderstanding was pretty influential in a lot of my thoughts above. Knowing that definitely gives me much more confidence about using webonyx/graphql-php as a foundation and helps mitigate my concerns about the two goals being in conflict.

The other key takeaway for me is that you're willing to deviate from the reference implementation for spec-compliant features that cannot be implemented on top of the library's current API.

Your point about creating new features in separate projects that build on this, rather than in this project, also makes a lot of sense to me.

Overall everything you said makes sense and is very encouraging to me as a user of the library.

@nagledb nagledb closed this as completed Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants