Inconsistent definition of value nodes

[Cito]

I noticed that "value nodes" are defined differently in language/ast and language/predicates:

export type ValueNode =
  | VariableNode  // <-- missing in isValueNode
  | IntValueNode
  | FloatValueNode
  | StringValueNode
  | BooleanValueNode
  | NullValueNode
  | EnumValueNode
  | ListValueNode
  | ObjectValueNode;

export function isValueNode(node: ASTNode): boolean %checks {
  return (
    node.kind === Kind.INT ||
    node.kind === Kind.FLOAT ||
    node.kind === Kind.STRING ||
    node.kind === Kind.BOOLEAN ||
    node.kind === Kind.NULL ||
    node.kind === Kind.ENUM ||
    node.kind === Kind.LIST ||
    node.kind === Kind.OBJECT ||
    node.kind === Kind.OBJECT_FIELD   // <-- missing in ValueNode
  );
}

Shouldn't these two definitions be consistent with each other?

[mjmahone]

@Cito would you be interested in putting up a PR to fix this? If not I can take it.

It seems like you're correct, unless @IvanGoncharov intentionally wanted isValueNode to mean is a constant value. We may want to switch the naming to better match the type though.

[IvanGoncharov]

It seems like you're correct, unless @IvanGoncharov intentionally wanted isValueNode to mean is a constant value. We may want to switch the naming to better match the type though.

It's a pure typo on my side. My idea was to match ValueNode exactly.

[Cito]

@IvanGoncharov I guess it happened because you copied the value nodes from kind,js. So maybe kind.js should be structured differently as well?

Update: Ok, I saw you addressed it already in PR #1462.

[IvanGoncharov]

@Cito Fixed in #1462 🎉
BTW. I saw your work on graphql-core-next and it's great.
Maybe you have some suggestions on what we can improve in graphql-js?

[Cito]

Thanks. I keep in touch with you when I have ideas or get feedback for improvement.