Fix #4 by making API equal to graphqls #11

DanielMSchmidt · 2019-01-21T16:10:14Z

Thank you @alexstrat for pointing this out. Would love to see a review from you if you have the time 👍

BREAKING CHANGE: All imports need to be `import {graphql}` instead of `import graphql` from now on Closes #4

BREAKING CHANGE: Its now schema, query instead of query, schema, so the inputs in all projects need to change Closes #4

Closes #4

Closes #9

Closes #5

alexstrat · 2019-01-21T18:59:46Z

src/reactive-graphql.ts

@@ -359,7 +359,7 @@ function resolveField(
      null // that would be the info
    );

-    if (resolvedValue instanceof Observable) {
+    if (isObservable(resolvedValue)) {


I'd do the same treatment to Promise (L366) and use a isPromise like done in the reference implementation

Hmm, I don't really see the benefit of duck typing instead of using instanceof, could you expand on your suggestion?

In some cases, the user thinks they are returning a Promise but it's not treated as it by reactive-graphql. For instance, when the user uses a lib that leverages other implementation of Promise than a native one.

For example, Sequelize uses bluebird and returns instance of bluebird.Promise that will not be instanceof global.Promise.

As Promise became am es6 standard, this case have less and less chance to happen, but still.

However, it is even more true for Observable, as many implementations exist and cohabit.

I was not aware that these implementations cohabit 🤔

alexstrat · 2019-01-21T19:03:20Z

src/reactive-graphql.ts

@@ -67,12 +68,24 @@ function isFieldWithResolver(

 export default function graphql<T = object>(


In the reference implementation:

graphql high-level API accept both DocumentNode and String, and its implementation transform a string in a document before passing it to execute

execute accepts only DocumentNode and its implementation and its implementation is defined in an other file in execution folder

It could be an inspiration for renaming function, file and change function signatures.

alexstrat

👏 Thanks! Added few suggestions

README.md

alexstrat · 2019-01-21T19:18:42Z

src/reactive-graphql.ts

@@ -67,12 +68,24 @@ function isFieldWithResolver(

 export default function graphql<T = object>(
  schema: Schema,


graphql( schema: GraphQLSchema, requestString: string, rootValue?: ?any, contextValue?: ?any, variableValues?: ?{[key: string]: any}, operationName?: ?string )

Actually, 3rd parameter is rootValue in the reference implementation. If you add support for rootValue in the future while keeping an API similar to the reference signature, the API will have to be broken. I think you should add unused rootValue parameter now.

Yeah, this makes sense, I'll try to add it in this PR 👍

Co-Authored-By: DanielMSchmidt <[email protected]>

BREAKING CHANGE: Please add the root value as third argument, null or an empty object are great default values for this.

DanielMSchmidt · 2019-01-22T09:33:56Z

So, the root value is added, I'll merge this now, so we can get all of the great changes @alexstrat did out of the door quickly

mesosphere-frontend-ci · 2019-01-22T10:07:21Z

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

alexstrat · 2019-01-22T13:57:23Z

src/reactive-graphql.ts

+  rootValue?: any,
+  context: object = {},
+  variables: object = {}
+): Observable<{ data?: T; errors?: string[] }> {


Observable<ExecutionResult<TData>> // with: export interface ExecutionResult<TData = ExecutionResultDataDefault> { errors?: ReadonlyArray<GraphQLError>; data?: TData; }

Note the difference of type on the errors key, and indeed note that line 82, parse throwing GraphQLErrors, GraphQLErrors are pushed in errors.

Yeah that might have been a mistake, normally we throw the observable, we should do this here, too. I mean not being able to parse is a prime example for something that a client can not live without in contrast to errors in field resolvers where one could argue for partial delivery of the data tree.

Would you mind fixing this into a throwObservable? I can do it tomorrow, but it would be nice to fix this even faster

Given GQL spec, any request validation error should be reported in the list of “errors” in the response and the request must fail without execution.
I'm not sure if an error in the syntax should be understood as "validation error" and I'm not sure what "request fail" mean.

Anyway, in the reference implementation, they decided to add any SyntaxError to the list of errors and to resolve normally the result with the errors key.

Given this, while I understand your POV, I think that (appart the typing) the current implementation is correct and I'd let it as it. 🤷‍♂️

ok now I realize that every error is actually thrown away with the current implementation, so by consistency this one should the same. However, I think it would be more correct to aggregate errors in the errors array in the returned results => #13

Daniel Schmidt added 5 commits January 21, 2019 16:55

fix: align export with graphql convention

0359c36

BREAKING CHANGE: All imports need to be `import {graphql}` instead of `import graphql` from now on Closes #4

fix: change order of arguments to align with graphql

40ba47e

BREAKING CHANGE: Its now schema, query instead of query, schema, so the inputs in all projects need to change Closes #4

feat: allow strings as query input

4d94700

Closes #4

docs: add link to repository to package.json

eeb1c1e

Closes #9

fix: rxjs interoperability

46bdff4

Closes #5

alexstrat reviewed Jan 21, 2019

View reviewed changes

alexstrat approved these changes Jan 21, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

alexstrat reviewed Jan 21, 2019

View reviewed changes

alexstrat and others added 2 commits January 21, 2019 23:35

docs: add new import type

3b51044

Co-Authored-By: DanielMSchmidt <[email protected]>

feat: add support for root value

ecd7a78

BREAKING CHANGE: Please add the root value as third argument, null or an empty object are great default values for this.

DanielMSchmidt merged commit aa6aa0d into master Jan 22, 2019

DanielMSchmidt deleted the unify-inputs branch January 22, 2019 10:03

mesosphere-frontend-ci added the released label Jan 22, 2019

alexstrat mentioned this pull request Jan 22, 2019

reactive-graphql changed signature in v3 getstation/apollo-link-reactive-schema#1

Closed

alexstrat reviewed Jan 22, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4 by making API equal to graphqls #11

Fix #4 by making API equal to graphqls #11

DanielMSchmidt commented Jan 21, 2019

alexstrat Jan 21, 2019

DanielMSchmidt Jan 21, 2019

alexstrat Jan 22, 2019 •

edited

Loading

DanielMSchmidt Jan 22, 2019

alexstrat Jan 21, 2019 •

edited

Loading

alexstrat left a comment

alexstrat Jan 21, 2019 •

edited

Loading

DanielMSchmidt Jan 21, 2019

DanielMSchmidt commented Jan 22, 2019

mesosphere-frontend-ci commented Jan 22, 2019

alexstrat Jan 22, 2019 •

edited

Loading

DanielMSchmidt Jan 22, 2019

DanielMSchmidt Jan 22, 2019

alexstrat Jan 22, 2019 •

edited

Loading

alexstrat Jan 23, 2019

		@@ -67,12 +68,24 @@ function isFieldWithResolver(

		export default function graphql<T = object>(

		@@ -67,12 +68,24 @@ function isFieldWithResolver(

		export default function graphql<T = object>(
		schema: Schema,

Fix #4 by making API equal to graphqls #11

Fix #4 by making API equal to graphqls #11

Conversation

DanielMSchmidt commented Jan 21, 2019

alexstrat Jan 21, 2019

Choose a reason for hiding this comment

DanielMSchmidt Jan 21, 2019

Choose a reason for hiding this comment

alexstrat Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

DanielMSchmidt Jan 22, 2019

Choose a reason for hiding this comment

alexstrat Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

alexstrat left a comment

Choose a reason for hiding this comment

alexstrat Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

DanielMSchmidt Jan 21, 2019

Choose a reason for hiding this comment

DanielMSchmidt commented Jan 22, 2019

mesosphere-frontend-ci commented Jan 22, 2019

alexstrat Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

DanielMSchmidt Jan 22, 2019

Choose a reason for hiding this comment

DanielMSchmidt Jan 22, 2019

Choose a reason for hiding this comment

alexstrat Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

alexstrat Jan 23, 2019

Choose a reason for hiding this comment

alexstrat Jan 22, 2019 •

edited

Loading

alexstrat Jan 21, 2019 •

edited

Loading

alexstrat Jan 21, 2019 •

edited

Loading

alexstrat Jan 22, 2019 •

edited

Loading

alexstrat Jan 22, 2019 •

edited

Loading