New GraphQL implementation about lighthouse HOT 30 CLOSED

I can confirm this. The code quality of https://github.com/digiaonline/graphql-php is outstanding and I also did quite a few performance comparisons. I am the maintainer of https://github.com/drupal-graphql/graphql and we are already in the process of switching to it now :).

from lighthouse.

@olivernybroe yes we have, and our library performs well compared to the other implementations. In fact we are overall a bit faster than the other implementations.

I spent a week optimizing the parser, which used to be the slowest part, and now we perform well in parsing too.

We are also working on Relay support, see https://github.com/digiaonline/graphql-relay-php for more info.

from lighthouse.

@chrissm79 Could be nice if we switched to a driver-based solution, so we just have to implement an interface for making DigiaOnline's graphql work. Having that would also give us the power to actually have support for both of the graphql implementations, by just shifting the driver.

from lighthouse.

Disclaimer: I am the maintainer of the webonyx/graphql-php lib.

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

As for performance, it would be great to have some benchmarking project, since both libraries are quite similar in what they do. I started one at https://github.com/vladar/graphql-bench

For now it just benchmarks lexer, feel free to contribute new benchmarks. As for Lexer - the following statement doesn't seem to be true:

I spent a week optimizing the parser, which used to be the slowest part, and now we perform well in parsing too.

From the initial benchmarks it is obvious that your Lexer is still O(N^2). Here is a breakdown of results (you can easily reproduce them from the project above):

+-----------+----------------------------+--------+--------+------+-----+------------+-------------+-------------+-------------+-------------+----------+--------+-----------+
| benchmark | subject                    | groups | params | revs | its | mem_peak   | best        | mean        | mode        | worst       | stdev    | rstdev | diff      |
+-----------+----------------------------+--------+--------+------+-----+------------+-------------+-------------+-------------+-------------+----------+--------+-----------+
| BigOBench | benchWebonyx10TypesSchema  |        | []     | 2    | 2   | 1,778,208b | 3.837ms     | 3.888ms     | 3.888ms     | 3.938ms     | 0.051ms  | 1.30%  | 1.00x     |
| BigOBench | benchWebonyx100TypesSchema |        | []     | 2    | 2   | 3,867,136b | 39.267ms    | 39.283ms    | 39.283ms    | 39.300ms    | 0.017ms  | 0.04%  | 10.11x    |
| BigOBench | benchWebonyx200TypesSchema |        | []     | 2    | 2   | 6,191,808b | 77.249ms    | 77.615ms    | 77.616ms    | 77.982ms    | 0.367ms  | 0.47%  | 19.97x    |
| BigOBench | benchDigia10TypesSchema    |        | []     | 2    | 2   | 1,769,760b | 24.657ms    | 24.846ms    | 24.845ms    | 25.035ms    | 0.189ms  | 0.76%  | 6.39x     |
| BigOBench | benchDigia100TypesSchema   |        | []     | 2    | 2   | 3,858,696b | 1,416.791ms | 1,419.569ms | 1,419.563ms | 1,422.346ms | 2.778ms  | 0.20%  | 365.16x   |
| BigOBench | benchDigia200TypesSchema   |        | []     | 2    | 2   | 6,183,368b | 5,437.041ms | 5,456.733ms | 5,456.694ms | 5,476.425ms | 19.692ms | 0.36%  | 1,403.66x |
+-----------+----------------------------+--------+--------+------+-----+------------+-------------+-------------+-------------+-------------+----------+--------+-----------+

So even with relatively small SDL (200 types) you'll face with performance issues.

And that's another reason why duplication of libraries is something that makes me sad. We already walked some path with webonyx/graphql-php. Including such problems with the parser (which are obviously fixed) and many others. Now you start it over.

Also for anyone having issues with webonyx version, please let us know by filling issues and contributing! After all, that's how open source works %)

from lighthouse.

I'll start working on implementing this.

from lighthouse.

I have been following along with the development of https://github.com/digiaonline/graphql-php and i must say i am really impressed. It is now nearing v1 and has some major improvements over webonyx, as well as great support for the SDL: AST manipulation, partial parsing, schema validation...

The driver-based solution by @olivernybroe is a neat idea, but i feel like it would broaden the scope of this library too much. We would spend half our time writing adapters or re-implementing functionality that the underlying libraries have, not to mention possible compatibility issues.

We currently lean pretty hard on webonyx, and expose parts of it in our public API. Because of this, we would not be able to make the switch without major breaking changes and large-scale refactorings. Also, we will have to stop support for PHP 7.0.

All things considered, i would advocate to keep an eye on the further developments and strongly consider making the switch as part of our eventual v3

from lighthouse.

@Jalle19 I don't think that the lexer by itself can help avoiding fragmentation. It is not even a part of the public API. I think it only makes sense to extract Parser/AST and related utils. Even this project relies heavily on AST, not lexer/tokens.

from lighthouse.

@crisu83 Thanks for reaching out!!! Lighthouse is pretty heavily dependant upon graphql-php but I'm always open to considering alternatives! Quick question for you, do you have a way to handle N+1 issues in your project? For instance, graphql-php provides a Deferred class that works similar to a Dataloader and is a must have to resolve N+1 queries. Thanks @crisu83!!

from lighthouse.

@olivernybroe A driver option is a great idea! Not sure how much work is involved but it looks like it could really be worth it.

@crisu83 A slack invite would be great, thanks!!

from lighthouse.

@hailwood That issue would also be there if we were only running driver Y.
But I don't mean that we should maintain multiple drivers in Lighthouse, we should only maintain one driver, in the same way Laravel does.
So I for example really wanted to use another graphql implementation then the one provided by Lighthouse, I could create a package with that driver and then just add the package this way. If Lighthouse then changes the interface in the next version, then my package won't be supported in that version until I can meet the requirements of the interface.

from lighthouse.

@olivernybroe gotcha! Yep I'm good with that!

from lighthouse.

@olivernybroe awesome! Can’t wait to see what you make. :)

from lighthouse.

@vladar that does sound really promising. We would love to discuss this further with you. Can you hop into https://digiaopensource-slack.now.sh/ and we can carry on the conversation there?

from lighthouse.

Hello @chrissm79,

We have a similar Promise based approach built on top of react/promise. My colleague @hungneox can tell you more about it as he is the one who implemented it.

I've heard that people have had trouble implementing features like Schema Decorators on top of webonyx/graphql-php. Our library has been built from the start with extendability in mind, so we have pretty good support for custom types, custom directives, and custom validators. You should also be able to override most of the library by injecting your own implementation.

We also encourage developers to use Schema Definition Language over schemas programmatically, because we believe that SDL is the future of GraphQL.

I'd love to hear what you think about our implementation.

Best,
Christoffer

from lighthouse.

@crisu83 Sounds neat,
Have you guys done any performance testing compared to the two other graphql libraries out there?

A big plus I can see with using their implementation is that they have Relay package also, so we don't have to handle this, but only make a wrapper for Laravel and add our custom directives.

from lighthouse.

@chrissm79 @olivernybroe Let me know guys if you want that Slack invite.

from lighthouse.

@fubhy How are you finding the process of switching? A lot of work?

from lighthouse.

Slack invite here too thanks :)

from lighthouse.

@chrissm79 yeah, not sure too about how much work it requires. But I think we should aim for that and implement digiaonline that way.

from lighthouse.

While the clear benefit is that if either of the packages ends up unsupported it would be relatively easy to switch out for the other, I'm wondering what the benefit would be outside of this for being able to support both packages?

The underlying library seems like it should be something that lighthouse is built to take full advantage of rather than being restricted to generics?

I'm just a bit concerned that we may end up spreading the package to thin in terms of support or ending up with a "we can't do X because driver Y can't support that event though drivers A, B, and C could" scenario.

Happy to be wrong, just voicing some concerns.

from lighthouse.

@chrissm79 @olivernybroe Here's the link, it's only valid for 24 hours so be quick.
https://join.slack.com/t/digiaopensource/shared_invite/enQtMzYzMTU5NzkzMTg0LTljMjBlOTgwM2RmMWI2NTViNmVmMDc3ZDIzZjQ4MTAwYzIyYThmNmFhYTkzNjY2NDdkN2Y5ZjU4YjExZDcxMmY
After you've accepted the invite, please join the #graphql-php channel.

from lighthouse.

@crisu83 thanks for the invite.

@hailwood I get your concern, but I still think we should do it. However I think we should be clear that our driver interface should require all the features we need, and then it's the drivers job to meet our criteria. Of course this means some drivers won't work as they e.g. Don't have custom directives or relay support. But we wouldn't be able to use the drivers which are missing our required features anyway.

from lighthouse.

@olivernybroe What happens if a user wants to submit a PR later to add a new feature, but only driver X supports it? Would it be that users responsibility to first submit a PR to driver Y to add support for that feature to that driver before submitting the PR to lighthouse?

from lighthouse.

@vladar on what version of PHP did you run your benchmarks? We do have a benchmark project, but unfortunately I’m travelling so I don’t have the link. @fubhy maybe you do?

from lighthouse.

@crisu83 PHP 7.2.3 but it doesn't matter much, because it is a question of algorithmic complexity. O(N^2) is still O(N^2) on any version. You can run those yourself.

from lighthouse.

Hey @vladar thank you for reaching out. I do agree that a single library would be the best.

There is some really great stuff in the digiaonline implementation that would be really nice to have. If we can bring it all together that would be awesome!

How do you feel about making a switch to PHP 7.1? I really like the strong type hints that it provides. Not having them in your implementation bit me a few times.

from lighthouse.

@spawnia The current version (0.12.x) is the last one supporting PHP5. We are in the process of converting the library to PHP7.1 In fact, we added a note in UPGRADE.md about the switch.

Also, we still consider moving towards a more static model where you need fewer closures and string-based mappings. The main reason it was not the case in the past was maintenance. It was way easier to follow reference implementation this way and port new features.

Now that things settle down we can consider some changes to make the library closer to how things are usually done in PHP. In general, we are quite open to justified changes, so feel free to post your ideas and suggestions on GitHub.

from lighthouse.

@vladar thanks for making benchmarks on the lexer performance, it is now obvious that our implementation is not ideal. I'm thinking it could be a good idea to factor out your lexer to a separate library, it could be a first step in avoiding fragmentation while still having multiple library implementations. The lexer is IMO a necessary evil in any GraphQL library and it indeed does not make sense to duplicate efforts in that area.

from lighthouse.

I agree with @vladar on this one.

from lighthouse.

As of now, i do not see a reason that would force us to make a switch away from webonyx/graphql-php. I also do not envision myself having the time to make such a switch, so i am closing this for now.

If someone wants to port over Lighthouse and can show that the result is better then what we have now, we can reopen this issue.

from lighthouse.