Make arguments accecpt number|boolean|string|object by iamchenxin · Pull Request #844 · facebook/relay

iamchenxin · 2016-02-16T17:39:51Z

Please review me.
Template commit for review.
This fix just covert the input identifyingArgValue(string|number|object) in getDataID and putDataID to a string.
Because GraphQL is already a type system ,so the number is covert to plain string,do not use maker to distinguish from string.
The object is covert to something like JSON,but with sorted key-value.

At this time , i m a bit confused by Object args, maybe should fix more file to accept this.
at now unit test stop at RelayQLAST.js:435.
The failed unit test is object arg

query{
        usertemp(tuple:{name:"tom",age:18}){ id }
}

worked in raw GraphQL server,but failed in relay. So this should be an additional type check error in RelayQLAST?
Continue tomorrow.

steveluscher · 2016-02-16T19:12:38Z

src/query/RelayFragmentPointer.js

+    let argType = typeof identifyingArgValue;
    invariant(
-      typeof identifyingArgValue === 'string' || identifyingArgValue == null,
+      typeof argType === 'string' || identifyingArgValue == null


Since your aim is to support every type of argument, you should simply be able to remove this invariant.

iamchenxin · 2016-02-17T02:30:38Z

@steveluscher Thank you,i will fix them all this evening.

iamchenxin · 2016-02-17T16:05:36Z

@steveluscher Need one more day to complete this.
Find that acceptting objects need to modify babel-relay-plugin.
Seems at now Relay makes the argument as an array:
(first:5 ) ->

calls: [{
    kind: 'Call',
    metadata: {},
    name: 'first',
    value: {  kind: 'CallValue',   callValue: 5}
},],

I think maybe the object-arg would be placed as a value for key 'callValue' ?

From RelayQLPrinter.js#L466 and RelayQLAST.js#L361 ,it seems there is no ObjectType support at all level RelayQLArgument.
Will add them tomorrow, maybe will modify another files which interactive with RelayQLArgument.

iamchenxin · 2016-02-17T18:40:33Z

@steveluscher Need help , When i use t.valueToNode from babelAdapter , Pass a number type to it still return something like {"type":"Literal","value": .
How could i make it return a numberType. What implement the valueToNode use.

Its my first time to look into a babel plugin,so there are so much things i do not known.
I search the modules of babel-relay-plugin find a function Literal(node) in babel's inferers.js

function Literal(node) {
  var value = node.value;
  if (typeof value === "string") return t.stringTypeAnnotation();
  if (typeof value === "number") return t.numberTypeAnnotation();
  if (typeof value === "boolean") return t.booleanTypeAnnotation();
  if (value === null) return t.voidTypeAnnotation();
  if (node.regex) return t.genericTypeAnnotation(t.identifier("RegExp"));
}

And i search the babel-handbook ,find some function like t.numberLiteral.

Go to sleep,see you tomorrow,thanks.

Seems i have wrong typing .. t.valueToNode is okay( return number type) , will recheck this tomorrow.

josephsavona · 2016-02-18T17:21:47Z

src/store/RelayRecordUtil.js

+    }
+
+  }
+};


how is this different than the existing stableStringify function?

@steveluscher ! Oh! i havent notice this function stableStringify .
Its my fault, i thought you let me rename my function to stableStringify , how stupid am i.

Need to sleep, will check that function tomorrow. and fix it

iamchenxin · 2016-02-18T17:59:37Z

@steveluscher Almost done. and need some help.
Did not find the function to let plugin generate Array [] . So i template write [] as key-value RelayQLPrinter.js#L556. Need to modify it tomorrow evening.
And i also a bit confused where to test the getDataID for new types.

steveluscher · 2016-02-19T02:31:11Z

scripts/babel-relay-plugin/src/RelayQLPrinter.js

+      }
    }

+    // todo array!


Does one of objectify or codify already do most of this?

Yeah, objectify already implements most of printObject. Let's just change the core line:

properties.push(property(key, t.valueToNode(value)));

To check the type of value and recurse if it is a nested object/array. Note that any helper functions can be moved out of the class definition to the bottom of the file.

iamchenxin · 2016-02-20T20:05:18Z

@josephsavona @steveluscher
Maybe i should learn jest first, i do unit test whole night by hand, have no idea why stableStringifyTmp.js be mocked by some test files.(but when i touched that file,it suddenly can require the real stableStringifyTmp.js). (temporary rename my function to stableStringifyTmp ,later,i should check if it is work right by using stableStringify).

Now the only failed test is this:

 FAIL  src/traversal/__tests__/printRelayOSSQuery-test.js (4.189s)
● printRelayOSSQuery › roots › it prints object call values
  - query PrintRelayOSSQuery($query_0:CheckinSearchInput!){checkinSearchQuery(query:$query_0){query}}
    to equal
    query PrintRelayOSSQuery($query_0:CheckinSearchInput){checkinSearchQuery(query:$query_0){query}}

CheckinSearchInput! vs CheckinSearchInput ( should check this,tomorrow)

And i have not set the webstorm right for eslint, so i will modify the code style later by hand,when i finish them. (i need to search how to get webstorm load the eslint plugins ?).

josephsavona · 2016-02-20T23:25:44Z

scripts/babel-relay-plugin/src/RelayQLAST.js

+    case 'ListValue':
+      return inputObject.values.map(v=>
+        getInputObjectValue(v)
+      );


could simplify with return inputObject.values.map(getInputObjectValue)

…and need to write unit test for new functions

…bject

Make all identifyingArgValue to `mixed type`. And delete some outside `not null check` for call stableStringify,to avoid get some magic result between files.

iamchenxin · 2016-02-26T06:04:03Z

@kassens thank you , rebased

iamchenxin · 2016-02-29T05:12:11Z

@josephsavona should this need reimport? it rebased after import.

josephsavona · 2016-02-29T17:02:15Z

Yup, i'll rebase internally.

josephsavona · 2016-03-02T19:35:44Z

@facebook-github-bot import

facebook-github-bot · 2016-03-02T19:39:34Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

josephsavona · 2016-03-02T23:48:06Z

@iamchenxin Thanks so much for working on this. When I imported and reviewed the code, I realized that all of the call sites of forEachRootCallArg were actually doing the same logic to convert the value to a key. Normally I'd make edits on the original PR, but in this case the changes ended up being quite extensive. I've created two new PRs that build on your implementation in #894 and #895, so I hope you don't mind if I close this PR and continue discussion there. Thanks again for doing so much work on this!

Summary:This is an expanded portion of just the plugin changes from [#844](#844). The goal of that PR is to support arbitrary values in identifying arguments. Part of that means supporting input objects, which we didn't fully support in the plugin (we only allowed the value of an input object argument to be a variable, not a literal). Changes: - Support parsing and printing literal InputObject expressions. - Clear error message when variables appear nested in ways that the Relay runtime cannot interpret. - Tests for the above. Closes #894 Reviewed By: yungsters Differential Revision: D3002163 Pulled By: josephsavona fb-gh-sync-id: d7708b36bc087bdd744c4d027cf4b888e8350798 shipit-source-id: d7708b36bc087bdd744c4d027cf4b888e8350798

Summary:Note: this is inspired by and partially based on iamchenxin's work in #767 and #844. Thanks for the head start! Relay currently assumes that identifying argument values are strings (numbers *sort* of work, but not really). This builds on #894 (which added support to the plugin for parsing/printing literal InputObjects) by allowing identifying arguments to be basically anything - boolean, number, string, or array/object of the the same. Key changes include: - Change the `CallValue` type from mixed to an explicit list of the supported types - Change `forEachRootCallArg` to return both the literal JS value of the argument as well as a serialized key - Change all callers of `forEachRootCallArg` (and some places that manually inspected the identifying arg) to correctly choose between the identifying argument value (i.e. when constructing a query with it) or the identifying argument key (for use with `RelayRecordStore.{putDataID,getDataID}`). - Added tests that the writer correctly creates root records with numeric/object identifying argument values. Closes #895 Reviewed By: yungsters Differential Revision: D3003201 Pulled By: josephsavona fb-gh-sync-id: 43ffbbd37e8d2bd8c0abe2cb792ad8959efb7a42 shipit-source-id: 43ffbbd37e8d2bd8c0abe2cb792ad8959efb7a42

iamchenxin · 2016-03-03T12:57:15Z

@josephsavona Its okay. Hope i did not add some hidden bugs in , and will keep a watchful eye on them.

Summary:This is an expanded portion of just the plugin changes from [facebook#844](facebook#844). The goal of that PR is to support arbitrary values in identifying arguments. Part of that means supporting input objects, which we didn't fully support in the plugin (we only allowed the value of an input object argument to be a variable, not a literal). Changes: - Support parsing and printing literal InputObject expressions. - Clear error message when variables appear nested in ways that the Relay runtime cannot interpret. - Tests for the above. Closes facebook#894 Reviewed By: yungsters Differential Revision: D3002163 Pulled By: josephsavona fb-gh-sync-id: d7708b36bc087bdd744c4d027cf4b888e8350798 shipit-source-id: d7708b36bc087bdd744c4d027cf4b888e8350798

Summary:Note: this is inspired by and partially based on iamchenxin's work in facebook#767 and facebook#844. Thanks for the head start! Relay currently assumes that identifying argument values are strings (numbers *sort* of work, but not really). This builds on facebook#894 (which added support to the plugin for parsing/printing literal InputObjects) by allowing identifying arguments to be basically anything - boolean, number, string, or array/object of the the same. Key changes include: - Change the `CallValue` type from mixed to an explicit list of the supported types - Change `forEachRootCallArg` to return both the literal JS value of the argument as well as a serialized key - Change all callers of `forEachRootCallArg` (and some places that manually inspected the identifying arg) to correctly choose between the identifying argument value (i.e. when constructing a query with it) or the identifying argument key (for use with `RelayRecordStore.{putDataID,getDataID}`). - Added tests that the writer correctly creates root records with numeric/object identifying argument values. Closes facebook#895 Reviewed By: yungsters Differential Revision: D3003201 Pulled By: josephsavona fb-gh-sync-id: 43ffbbd37e8d2bd8c0abe2cb792ad8959efb7a42 shipit-source-id: 43ffbbd37e8d2bd8c0abe2cb792ad8959efb7a42

facebook-github-bot added the CLA Signed label Feb 16, 2016

steveluscher reviewed Feb 16, 2016
View reviewed changes

josephsavona reviewed Feb 18, 2016
View reviewed changes

steveluscher reviewed Feb 19, 2016
View reviewed changes

josephsavona reviewed Feb 20, 2016
View reviewed changes

iamchenxin added 13 commits February 26, 2016 04:42

fixed int to string,do not complete object to string

26b7599

auto merge

f6648fb

uncompelted,need to find the right function for generate babel-array,…

0217002

…and need to write unit test for new functions

printRelayOSSQuery-test.js test failed

d35195b

fixGraphQLFragment

8f9b446

rebase to master

99c2c81

change type for to mixed

53c5fef

add some special cases for array-holes

040792c

make public function stableStringify has a mixed arg => (input: mixed)

1b2e12c

add some holes-array tests

4042495

number type arguments is okay,but seems something wrong for complex o…

fc7cafc

…bject

Why fn(identifyingArgValue) will auto stringify identifyingArgValue ?

bfbe5a4

Make all identifyingArgValue to mixed type.

9e92617

Make all identifyingArgValue to `mixed type`. And delete some outside `not null check` for call stableStringify,to avoid get some magic result between files.

iamchenxin force-pushed the fixGraphQLFragment branch from 3e217b7 to 9e92617 Compare February 26, 2016 04:48

Add unit test for forEachRootCallArg.js

906f9a8

This was referenced Mar 2, 2016

babel plugin: support input object literals #894

Closed

Non string identifying argument values #895

Closed

josephsavona closed this Mar 2, 2016

ebarahona mentioned this pull request Jun 22, 2022

[Snyk] Fix for 21 vulnerabilities ebarahona/relay#52

Open

+                  }
+                }
+              };

Conversation

iamchenxin commented Feb 16, 2016

Uh oh!

steveluscher Feb 16, 2016

Choose a reason for hiding this comment

Uh oh!

iamchenxin commented Feb 17, 2016

Uh oh!

iamchenxin commented Feb 17, 2016

Uh oh!

iamchenxin commented Feb 17, 2016

Uh oh!

josephsavona Feb 18, 2016

Choose a reason for hiding this comment

Uh oh!

iamchenxin Feb 18, 2016

Choose a reason for hiding this comment

Uh oh!

iamchenxin Feb 18, 2016

Choose a reason for hiding this comment

Uh oh!

iamchenxin commented Feb 18, 2016

Uh oh!

steveluscher Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

josephsavona Feb 19, 2016

Choose a reason for hiding this comment

Uh oh!

iamchenxin commented Feb 20, 2016

Uh oh!

josephsavona Feb 20, 2016

Choose a reason for hiding this comment

Uh oh!

iamchenxin commented Feb 26, 2016

Uh oh!

iamchenxin commented Feb 29, 2016

Uh oh!

josephsavona commented Feb 29, 2016

Uh oh!

josephsavona commented Mar 2, 2016

Uh oh!

facebook-github-bot commented Mar 2, 2016

Uh oh!

josephsavona commented Mar 2, 2016

Uh oh!

iamchenxin commented Mar 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants