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.