Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow rangeBehaviors to be defined as a function #1054

Closed

Conversation

@xuorig
Copy link
Contributor

xuorig commented Apr 16, 2016

New PR for #639
Fix for #615, proposed in #538

Allows rangeBehaviors to be defined as a function that receives the connection arguments and returns one of GraphQLMutatorConstants.RANGE_OPERATIONS.

changes summary:

  • added a getRangeBehavior module that will return a rangeBehavior depdending on rangeBehaviors config and calls passed by argument.
  • changed RelayQuery's getRangeBehaviorKey to getRangeBehaviorCalls so it can be reused by a function or plain object rangeBehaviors config.
  • Added some documentation on using a function as rangeBehaviors config.
  • Modified todo example to use a function.

const flattenRelayQuery = require('flattenRelayQuery');
const forEachObject = require('forEachObject');
const nullthrows = require('nullthrows');
const inferRelayFieldsFromData = require('inferRelayFieldsFromData');
const intersectRelayQuery = require('intersectRelayQuery');
const getRangeBehavior = require('getRangeBehavior');

This comment has been minimized.

Copy link
@josephsavona

josephsavona Apr 16, 2016

Contributor

nit: alpha ordering

@@ -24,12 +24,14 @@ import type RelayQueryTracker from 'RelayQueryTracker';
const RelayRecord = require('RelayRecord');
import type {Variables} from 'RelayTypes';
const {REFETCH} = require('GraphQLMutatorConstants');
import type RelayRecordStore from 'RelayRecordStore';

This comment has been minimized.

Copy link
@josephsavona

josephsavona Apr 16, 2016

Contributor

unused?

@xuorig xuorig force-pushed the xuorig:range-behaviors/function-definable branch from ae08a0f to 7f2a523 Apr 16, 2016
@xuorig
Copy link
Contributor Author

xuorig commented Apr 16, 2016

addressed your comments @josephsavona

Not sure about the CI Failure ?

@xuorig xuorig force-pushed the xuorig:range-behaviors/function-definable branch 4 times, most recently from 9930e39 to 9795d94 Apr 16, 2016
@xuorig
Copy link
Contributor Author

xuorig commented Apr 16, 2016

Somehow I'm getting this Flow type error on master too ¯_(ツ)_/¯

@xuorig xuorig force-pushed the xuorig:range-behaviors/function-definable branch from 9795d94 to c66dd5d Apr 21, 2016
@xuorig
Copy link
Contributor Author

xuorig commented Apr 21, 2016

Rebased and got 🍏, I know this is a fairly large PR, but any comments ? cc @josephsavona


return this.getCallsWithValues().filter(arg => {
return this._isCoreArg(arg);
});

This comment has been minimized.

Copy link
@josephsavona

josephsavona Apr 21, 2016

Contributor

__rangeBehaviorKey__ was previously used to memoize the result of this field. Let's either removing that property altogether or renaming it to __rangeBehaviorCalls__ and restoring the memoization (preference toward the latter).

*/
getRangeBehaviorKey(): string {
* An array of calls excluding Core args to use with rangeBehavior
* functions.

This comment has been minimized.

Copy link
@josephsavona

josephsavona Apr 21, 2016

Contributor

maybe keep the explanation of what arguments are excluded

const rangeBehavior = getRangeBehavior(rangeBehaviors, calls);
expect(rangeBehavior).toBe('append');
});
});

This comment has been minimized.

Copy link
@josephsavona

josephsavona Apr 21, 2016

Contributor

nit: it would be good to test a non-matching case too

@josephsavona
Copy link
Contributor

josephsavona commented Apr 21, 2016

@xuorig sorry for the delay in review. this looks pretty solid, just a few nits and we should be good to go.

@xuorig xuorig force-pushed the xuorig:range-behaviors/function-definable branch 2 times, most recently from 3529c5d to f8311ec Apr 21, 2016
@xuorig xuorig force-pushed the xuorig:range-behaviors/function-definable branch from f8311ec to 282ea24 Apr 21, 2016
@xuorig
Copy link
Contributor Author

xuorig commented Apr 21, 2016

addressed your comments @josephsavona :)

@josephsavona
Copy link
Contributor

josephsavona commented Apr 21, 2016

@josephsavona
Copy link
Contributor

josephsavona commented Apr 21, 2016

thanks!

@facebook-github-bot
Copy link

facebook-github-bot commented Apr 21, 2016

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

@ghost ghost closed this in 56e52d3 Apr 28, 2016
@josephsavona
Copy link
Contributor

josephsavona commented Apr 28, 2016

@xuorig thanks for this!

@tim-field
Copy link

tim-field commented Oct 17, 2016

Hello, is this likely to be released soon ? Looks like a really useful feature.

@josephsavona
Copy link
Contributor

josephsavona commented Oct 20, 2016

This is included in the latest release.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.