Graphql-js: Proposal: refactor for RxJS compatible

[xareelee]

I'm starting to build another version of GraphQL library using RxJS instead of Promise for reactive programming.

I won't add Rx to this library, just build another library and reuse most of code this library provides (with module dependency).

To minimize the efforts of maintenance, I need to refactor this original graphql library to extract Promise-associated code into separated files, and export some functions to make them importable for reuse. It won't change any behavior which this library provides. Just separate the concern of event handler (Promise, Rx, and so on) from the GraphQL design.

At a glance, I think I just need to refactor the file graphql/execution/execute.js mainly. The goal is make Promise-based functions become callback-based (Promise-free), and turn them into Promise-based in separated files (and also I can turn them into Rx-based easily).

[DxCx]

Hi,

Already done by me and rejected by @leebyron.
See #502.

Ill probably release in the future an addon libraray which will hook graphql library and add observable support.

[xareelee]

@DxCx and @leebyron,

It's different from #502 and my approach. I agree with @leebyron's comment: _"not taking a dependency on a large library"_.

I'm not planing to introduce Rx or any other dependency into graphql-js. I'll just refactor existing functions to use a specific callback design, and turn them into Promise.

I just wrote an article to explain what it is and how I'll do. Please refer to my article: A better async/reactive function design in JS (or any other languages) — the Universal Currying Callback (UCC) Design

Simply put, I'll make the functions become callback-based and turn them back to Promise in a separated file for decoupling. The callback-based functions would be easily turned into Promise, Rx or any other async handler system (bacon.js, highland.js, sodium, xstream, etc.).

// A universal currying callback (UCC) function
function graphql$UCCF = 
  (schema, requestString, rootValue, contextValue, variableValues, operationName) => 
  (...callbacks /* please refer to the article */) => {
  ...
};
// You can use it directly
graphql$UCCF(schema, query)(...callbacks);

// Or use it as Promise flovar (We'll still provide `graphql()` as Promise-based)
const graphql = promisify(graphql$UCCF);
graphql(schema, query).then(...);    // still the same API as the original design

// Or Rx flavor (need to install `Rx` and write your own rxify() to turn a UCC function into Rx.Observable which could be done by a new module)
const graphqlRx = rxify(graphql$UCCF);
graphqlRx(schema, query).subscribe(...);

It doesn't break any design or introduce any new dependency. Just refactor existing functions to UCC design and make those UCC functions be public.

The UCC design has the minimum reactive feature but not functional (not as a functor/monad to support map/flatMap or other FP methods).

[xareelee]

@DxCx and @leebyron,

I open a PR on #649 for src/graphql.js to demonstrate how to do with the UCC design.

If this is OK, I'll keep refactoring.

I'm not familiar with flow type system. Any help to complete the type is welcome. :)

[leebyron]

Thanks for your investigation on this, but I don't quite understand the purpose in how it creates value for this library.

I think if the refactoring results in a much simpler implementation, then I would consider the changes in a PR, but the current PR attached seems to create complexity without adding value.

[DxCx]

@xareelee after seeing what you meant, i like the idea, or the approach in general.
as lee said, it won't fit in here the same reason my solution did not fit.
but just out of curiosity, did you review the internal implementation of execute?
im not sure it can really work because inside the execution engine, there are operations that are running in parallel, and resolving algorithm is like a populating tree. i'll be happy to hear how do you plan to solve this..

also, if i got it right, the resolver functions will not be able to just return promise or observable, so integration to existing libraries (mongoose for example) will require more adapters..
did i get it right?

[xareelee]

@DxCx,

Thanks for your reply.

but just out of curiosity, did you review the internal implementation of execute?
im not sure it can really work because inside the execution engine, there are operations that are running in parallel, and resolving algorithm is like a populating tree. i'll be happy to hear how do you plan to solve this..

The universal currying callback (UCC) design is just the core part (or prototype) of reactive programming without Functor/Monad. It fulfills the minimum requirement for reactive programming. You just build the pipeline end-to-end (from the source to the subscriber, in reactive way).

You've already built the graphql-rxjs using Rx approach. The underlying mechanisms are the same between the UCC design and Rx. The UCC design is just the abstraction of reactive programming, and implemented by callback-based which allows you convert it to any FRP library or Promise (will lose some contexts or continuous events).

I just finished refactoring graphql() and execute() with passing the tests, and start to refactor executeFields() and executeFieldsSerially(). I'll update the progress if you want to know.

also, if i got it right, the resolver functions will not be able to just return promise or observable, so integration to existing libraries (mongoose for example) will require more adapters..
did i get it right?

I'm not sure about your case. Converting to Promise or observable is just one line code (calling an adapter function). Usually, you just need one adapter to convert all kinds of the reactive-callback-based functions to observables. It depends on how to subscribe the events.

[DxCx]

cool @xareelee yeah i'll be happy to hear about it.

about the second question, what i meant, is that if today i have an existing scheme, with 40 resolvers already implemented. all of them are returning promises.
will i have to go one by one to convert them to UCC style return?

[xareelee]

Hi @DxCx,

No. I do not attempt to change the API of the current graphql-js. You can still use my version of graphql-js with the same code.

What I do is exporting the reactive-callback-based functions to be public, and the users will have the chance to turn them be observable for FRP.

However, if your resolvers are Promises, the subscribers will only receive one value, not values over time.

The resolver-end of graphql-js will act like Rx.Subject which is both subscriber and observable. Implementation details are not done yet.

[xareelee]

@leebyron,

I think if the refactoring results in a much simpler implementation, then I would consider the changes in a PR, but the current PR attached seems to create complexity without adding value.

OK, once I finish the refactoring, I'll let you know. Currently, I think the results might be simpler than using Promise.

[xareelee]

@leebyron,

I have reviewed the code in the execute.js last night. I'd mention something I observed:

First, graphql-js heavily relies on try...catch... pattern to handle errors. The throw-try-catch has been considered as anti-pattern. The throw expression is a side-effect which can't be observed directly in the return values.

There are two ways for error handling:

1. exception: use throw to terminate the program for severe errors
2. error: return error objects to tell outside the abnormal results for appropriate handling

If you just want to report the errors, not attempt to terminate the program, you should consider to use return or use callback to pass the error to the outside rather than throw.

// return style
function returnResultOrError(input) {
  let result, error;
  ...

  if (error)  return { error };
  else return { result };

  // or
  return { result, error };
}

// async callback style (node style)
function runResultOrError(input, callback) {
  let result, error;
  ...

  if (error)  return callback(error, null);
  else return callback(null, result);

  // or
  return callback(error, result);
}

Second, I found some side-effects using mutable objects (e.g. context.errors). This should also be considered to avoid.

Third, in my opinion, graphql-js is a data handling (querying) library which should be considered using pure functions as possible (one input, one observable result).

I'll use pure functions and reactive-callback functions as possible when refactoring. This would make the most part of execute.js be re-written.

[leebyron]

Thanks for your opinions, @xareelee.

I would be open to any pull requests which implement any of these ideas as long as they are simplifying and do not introduce considerable overhead.

For now I will close this issue since it is no longer directly actionable. Feel free to continue discussing if necessary.

[andykais]

@xareelee, do you have any plans to continue with this refactoring? I see that your fork has not been worked on in some time but I still would love to see observables included within this library

[DxCx]

There is a library already out and working:
https://www.npmjs.com/package/graphql-rxjs
Feel free to contact me for support.

[andykais]

yes I saw that, but I am worried about the maintainability of a fork of the main graphql library. If you decide to stop working on that library, it is likely to cease being worked on entirely. I also worry that it is likely to miss out on future features like @defer

[DxCx]

well, i am planing to maintain it and release versions along graphql.
also, i'm experimenting towards support reactive directives (@defer/@live/@stream)

[xareelee]

@andykais & @DxCx Sorry for the delayed response. I've already done the refactoring work in January using both RxJS and most.js.

The problem I encounter was performance and memory issue. Using RxJS instead of Promise-based will be much slower than Promise, and it hit the memory issue when doing benchmark. However, using most.js will be a little faster than Promise-based when querying but a little slow when mutating.

I'll push my code recently to let you review when I'm available.

Another thing is what reactive library would be used in this repo. Do we need to develop our own reactive library specifically for graphql.js or rely on an external dependency? Will Observable be the standard of ECMAScript in the future?

[xareelee]

@andykais & @DxCx

I just make a PR for review about reactive graphql in https://github.com/graphql/graphql-js/pull/809.

Check it out and give me some feedback. 😄

[DxCx]

hey @xareelee sounds interesting i'll take a look at that,
also note that im maintaining a fork package named graphql-rxjs which is just about that..
using it for a while now and already experimenting with reactive directives.. (@defer/@live)