Planned changes in 3.0

[wravery]

I'm starting a list of improvements I'd like to make for 3.0. Many of these will break backwards compatibility, so I hope to do as many of them as possible in a single major version release.

Adopting C++17

• Move to PEGTL master/3.0.0 (not yet officially released, also requires PR Reflect PEGTL api change .content() -> .string()/.string_view() #43)
• Use std::variant for response::Value
• Replace std::unique_ptr for nullable fields with std::optional

Schemagen improvements

• Make NYI stub generation optional
• Split the class definitions/declarations into separate files. May also help with making the NYI stubs optional (i.e. decide whether or not to compile and link them individually).
• Improve error handling/reporting. Currently if there's a schema validation error it gives a very poor error message and requires attaching a debugger to track down what caused the error.
• Add command line options for things like defining a target directory. Also another way to make the NYI stubs optional.
• Change the names of the generated Mutation accessors to something like applyFieldName. Calling everything getFieldName feels strange when performing a mutation.

Miscellaneous/TBD

• Let the caller specify the std::launch policy for the Request::resolve top-level call.
• Replace resolver std::unordered_map with either a pre-sorted std::vector or generated if/else if blocks?
• Build a separate library target for the grammar/AST.
• Replace std::string with std::string_view where appropriate.
• Replace ast<std::string> with ast<std::string_view> or ast<std::vector<char>> to deal with small-string-optimization issues rather than reserving extra space when the string is too short.
• Replace the facebook::graphql namespace with just graphql for the sake of brevity.

Documentation

• Should add more documentation, either to the README.md or in sub-documents.

Postponed to 3.1

• Use a PIMPL pattern or CRTP instead of inheritance for more static polymorphism?
• Experiment with Protocol Buffers for response serialization, but maybe in a separate project like cppgraphiql.

[Sarcasm]

Your C++17 planned changes looks great!
Regarding the parser/AST representation.
I haven't look too much into it yet, but I feel like this could be a useful component of its own which some people might want to reuse, in place of https://github.com/graphql/libgraphqlparser.

A thought regarding the use of std::future for return values.
I have found the use of std::future impractical due to the lack of continuation support. I don't know how to solve this with what the STL offers but if you want to integrated a GraphQL service with an event loop it's difficult difficult to handle multiple concurrent requests without resorting to threads waiting on the futures.
I feel like a system based on callbacks/completion handlers, while less practical to use, would permit a better integration with such event loops. GraphQL seems to really benefit from an event loop, e.g. things like Dataloader rely on it for optimisation.

Regarding the namespaces.
I find the namespace very verbose, I don't know if it's possible to have just one namespace for the whole codebase, but that would simplify things IMHO.
Typically, the namespace facebook looks weird to me as GraphQL originated from here but is now more of an open standard.

Schemagen idea, I think it would be interesting to be able to fully customize the generated code namespaces, so the generated code can integrate well with an existing code base which want to add GraphQL support.
And not only the namespace, but the generated file paths, which sometimes takes the same layout as the namespaces.
e.g. for a namespace foo, the file FooSchema.h could be included with #include "foo/FooSchema.h",
and for a namespace foo::graphql some people may want to have it under foo/graphql/FooSchema.h.

[wravery]

Making the parser/AST available separately is something I've been considering. I could spin it off into a separate project, or even just separate it into a separate library which is independently consumable. Maybe more ambitious would be to send a PR to the PEGTL project's contrib directory with the GraphQLGrammar.* and GraphQLTree.* components, but I need to make sure that the copyright/license are compatible with redistributing it that way.

There's definitely room for improvement in the std::future usage. You can kind of simulate a non-blocking event loop instead of separate threads by calling wait_for with a 0-duration timeout and looping over the pending futures until they're all completed, but I don't think it's guaranteed not to block at all on each of those calls. That might just be the best option until some of the experimental concurrency improvements arrive in C++20.

I originally picked facebook::graphql::... to align with libgraphqlparser, but it's been a while since I replaced that dependency. I'll think about dropping the top level namespace in the 3.0 update.

[Sarcasm]

For the parser, a separately consumable library in the project sounds reasonable to me, because for now there are not too many dependencies.

Ah, thanks for the std::future suggestion, if the futures are consistently fulfilled by the event loop it is better than using one waiter thread per future.

[wravery]

Actually, I just realized the std::future suggestion won't work without modification. All of the std::async calls in GraphQLService.cpp use std::launch::deferred, and ultimately ModifiedResult<Object>::convert will block on a call to std::future::get. So even if you try to call wait_for with a 0-duration timeout, it will either return std::future_status::deferred, or you'll have to spin a separate thread and block it on the std::future::get call anyway to start it.

Of course, if you're going to spin additional threads to execute the request concurrently anyway you could wrap that in your own std::async call with std::launch::async, and then use the wait_for(0) approach on that future to still handle the requests in an event loop. But it might be better to build that in as an overload which changes the launch policy for the request in GraphQLService.cpp to make it use std::launch::async for at least the top level promise so it starts on its own.

[d-frey]

Just FYI: We just released version 2.8.0 of the PEGTL, I backported almost all of the features from the master to C++11. I might have broken the link in your README.md to the branch (2.7.x -> 2.x), but otherwise it should be compatible.

You may, however, rename the enumerators, etc. to make it also compatible with the master branch, as version 2.8.0 now supports both spellings for an easier transition. If you have any questions or need help just let us know.

[wravery]

But it might be better to build that in as an overload which changes the launch policy for the request in GraphQLService.cpp to make it use std::launch::async for at least the top level promise so it starts on its own.

This is what I did.

[wravery]

That's the last of the planned changes for 3.0. I'll go ahead and release it after a few days assuming no new issues crop up.