rangeBehaviors are too strict

[ooflorent]

v0.9 introduces a way to declare rangeBehaviors as a function (56e52d3). This is super useful and I cannot thank you enough for this! However, they introduces a new restriction:

// getRangeBehavior.js#L75
invariant(
  typeof behavior === 'string',
  'getRangeBehavior(): Expected range behavior for key `%s` to be a ' +
  'string, got `%s`.',
  call.name,
  behavior
);

This is an issue if your connection is defined like this:

enum PostOrder {
  DATE_ASC
  DATE_DESC
  TITLE_ASC
  TITLE_DESC
}

type Blog implements Node {
  id: ID!
  title: String
  posts(
    first: Int
    after: ID
    order: PostOrder
    since: Int
  ): BlogPostConnection
}

The invariant test may fail on valid arguments. The following calls would raises an error:

[
  // `order` holds an enum value.
  // Passing the value as a string is fine.
  // However, passing `null` would throws.
  {
    name: "order",
    value: null,
  },
  // `since` holds a timestamp.
  // Converting it to a string would result in an invalid query but numbers are allowed!
  {
    name: "since",
    value: 1465542134907,
  }
]

rangeBehaviors object notation isn't affected by this issue because calls values are serialized.

[ooflorent]

@xuorig Since you are the one who proposed this changed and implemented it, could you bring some insight about this issue? Particularly why is getObjectFromCalls denying non-string values?

[keithpitt]

I actually hit this problem myself. I looked around the supporting code, and I couldn't find a reason as to why it's there. The error message itself is actually the same as another in the same commit 56e52d3#diff-fe15bf16b6fdab1fc8ae96d4902f0f83R50 and 56e52d3#diff-fe15bf16b6fdab1fc8ae96d4902f0f83R77

My current theory is that it could be a copy/paste error (since the error message itself doesn't actually make much sense in this context). Maybe it's easier if we open a PR to remove that check and continue the discussion there?

[xuorig]

Strange I don't remember adding this check in my PR @ooflorent. This was possibly added on FB side when they imported, so I might be missing some context.

I agree that this really should not be there! We should either remove the check or look for undefined there probably...

[wincent]

This was possibly added on FB side when they imported, so I might be missing some context.

It was added by @josephsavona with the comment "lint/flow fixes", so maybe he recalls what the lint/flow errors were that prompted the change.

[josephsavona]

I remember that I simplified the PR a bit, which prompted some type-fixing. Good catch, the type of call.value should be mixed (here). Anyone interested in fixing in a PR ;-)?

[ooflorent]

@josephsavona: @xuorig has submitted a fix yesterday (#1216)