Graphql-js: Release v14.0.0

[leebyron]

Let's coordinate on releasing this final version, @IvanGoncharov and @mjmahone.

I believe #1382 is the final blocker based on previous discussions I've had with you all about what we'd like to include. If there's anything else we should have blocking this release let me know.

Next steps are to write up the full release notes with special mentions of breaking changes, then finally push the version bump and commit tag.

[IvanGoncharov]

@leebyron There is a couple issue I think we need to discuss:

1. After #1373 merged we still have a few related issues:
   
   
   
   1. validateSchema doesn't use nodes from new extensionASTNodes properties.
   
   
   2. extend scalar is not supported
   
   
2. I also think it's very critical to add validation of directives in SDL since as noted here no one validates them correctly at the moment. That will lead to a situation very similar to the recent Allow interfaces to have no implementors. Since spec defines set of validation rules but no one implements them. Probably we shouldn't enforce Directives Are Unique Per Location until https://github.com/facebook/graphql/issues/429 resolved but all other validation should be implemented e.g. require the schema to contain applied directives, correct directive arguments, etc.
3. Should we switch from IE9 to IE11 especially since IE9 & IE10 not fully supported by babel?
   Don't know a lot about ios but maybe it also worth to update.
   https://github.com/graphql/graphql-js/blob/f2070c8169617b949b17c1929d27e46454a1d90a/.babelrc.js#L7-L8
4. I think we should use this opportunity to clarify on deprecated functionality. I propose the following:
   
   
   
   1. Drop support for onOperation, onFragment, onField since they were deprecated two years ago.
   
   
   2. Explicitly say that following functions would be removed in 15.0.0 release isValidJSValue,isValidLiteralValue,introspectionQuery.
   
   
   3. Explicitly say that support for legacy SDL syntax would be removed in 16.0.0. Also, we can recommend all 3rd-party tools and libraries to stop supporting it by default and require a user to explicitly specify that he wants to use legacy SDL syntax through configuration/option.
   
   

[IvanGoncharov]

@leebyron I also forget to mention one more breaking change which is not super important but a huge quality of life improvement. parseLiteral should throw on invalid values similar to parseValue.
https://github.com/graphql/graphql-js/blob/f2070c8169617b949b17c1929d27e46454a1d90a/src/type/scalars.js#L56-L64
Same goes for enums:
https://github.com/graphql/graphql-js/blob/f2070c8169617b949b17c1929d27e46454a1d90a/src/type/definition.js#L1105-L1113

[leebyron]

@IvanGoncharov want to open a new issue for throwing from parseLiteral? Not sure I totally understand the use case and want to discuss further but don't want to hijack this issue

[leebyron]

I just put up PRs to address your point 4, @IvanGoncharov. Are you interested in working on point 1 or 2? I'll start looking into point 3.

[leebyron]

To follow up on point 3, changing "ie 9" to "ie 11" has no impact on compiled output (tested with diff -r), only removing both ie and ios entries causes any change - for example then const is not compiled to var - however I don't think this is something we want to do, so I'll suggest leaving that configuration as is for now.

[leebyron]

I created new tasks for the remaining issues you brought up and created a milestone so we keep track of them: v14.0.0 Milestone

If you (or anyone else who might be reading this? @mjmahone?) is interested in helping work through these that would be a huge help in getting this release out this week.

[alexmylonas]

Excuse my ignorance, but why you jumped from v0.13 -> v14? Is this misname or is this a common practice, just curious!

[jgcmarins]

@alexmylonas -> https://github.com/graphql/graphql-js/issues/1005

[alexmylonas]

Thanks for the response. Solid read and discussion

[PowerKiKi]

This issue should probably be part of milestone v14.0.0.

[leebyron]

Hey @IvanGoncharov and @mjmahone - let's wrap this one up, what do you think?

It seems like #1389 is the last remaining issue - what do we need to be okay with a v14 release and what portions of that could wait for a v14.1?

[mjmahone]

I'm onboard with cutting the 14.0.0 release. I don't think there's anything we're waiting for that can't be pushed to a 14.1 release, but I want to make sure we have consensus with @IvanGoncharov too.

[IvanGoncharov]

@leebyron As for #1389 we currently validating that:

• directive is known
• it used on correct location
• all directive args are known
• all required arguments specified
• no directive duplication on the same location (but without checking extends)
  So we not checking only:
  https://github.com/graphql/graphql-js/blob/master/src/validation/rules/ValuesOfCorrectType.js

My idea was to force developers to specify they use inside SDL so I think current state satisfy this. We can adapt ValuesOfCorrectType to SDL in 14.1 release since it's not breaking change just an additional validation.

There is two small issue I want to fix before 14.0.0 release:

• #1494
• And drop support for undefined as enum value since it taints not only enum values but also defaultValue and all code that work with values. 90% of this library doesn't handle undefined as valid value. For example any function that do schema transformation will unintentionally define default value for every arg simply by writing code like this:
  https://github.com/graphql/graphql-js/blob/f4dee2803c22f7e5fde913b9743cc6de345bb871/src/utilities/lexicographicSortSchema.js#L86-L91
  + by adding bunch of hasOwnProperty all over the place we introduce a lot of JS-specific code into implementation and bunch of test cases for almost every function.

I don't think anyone really using this feature but it creates a lot of inconcistency in this library.
I will prepare PR tomorrow morning and it the last thing I want to disscuss before 14.0.0

@leebyron If you have time it would be great if you could review PRs with breaking changes: https://github.com/graphql/graphql-js/pulls?q=is%3Apr+sort%3Aupdated-desc+is%3Aclosed+label%3A%22semver+major%22
To be sure that we didn't miss anything major before we freeze public API for v14.x.x.

[mjmahone]

And drop support for undefined as enum value

Given, as you've pointed out, that 90% of our code can't handle it as-is, I don't think it would be a real breaking change either way to do one of:

• drop support entirely (because no one could use it correctly anyways)
• do the hard work to actually support it

I'm really happy with the state things are in right now, but I will try to do the release tomorrow morning around 13:00 UTC to give time for all of us to comb over things one more time.

[IvanGoncharov]

@mjmahone Sound good 👍
Since it's UTC+3 for me I will have time to go over all open issues one more time to see that we didn't miss anything.
Do you need any help with release notes?
I can create a list of commits and group them into by bug/feature.
We also need to write short instruction about two ways how you can specifying directives for your SDL.

[mjmahone]

Alright, I'll do the release in ~10 minutes. @IvanGoncharov I know you've put up some last minute PRs: anything worth waiting for?

[IvanGoncharov]

anything worth waiting for?

@mjmahone No, I think we are good to go 🏁🏎
I think we could disscuss undefined as enum values beetween RC and 14.0.0 release.

Do you need any help with release notes?

[mjmahone]

@IvanGoncharov plan is to do a full 14.0.0 release, not an RC. This release has been baking for long enough, let's just rip off the band-aid.

I think I should be OK aggregating my previous RC release notes and the changes since then.

[IvanGoncharov]

@mjmahone Changes in directive validation can potentially break Relay or other Facebook tools. Based on previous experience I'm worried that it will provoke reverts.
Can you please ensure that it doesn't break anything at Facebook.
Maybe it will make sense to create intermediate RC for a couple of days and agree that we don't merge anything in master.

[mjmahone]

I'm responsible for merging it into Facebook's tools, so I'll make sure. Using an intermediate RC doesn't actually help though: we can't do the Relay upgrade to an RC, though I can test a bit with an RC and find some potential issues (can't merge it into trunk as long as it's an RC, though).

If we need to do a quick, follow-on patch release, we will, but I don't think Relay currently has much potential for directive validation issues, and if it does I have the ability to fix Relay forward.

[IvanGoncharov]

@mjmahone In that case, I'm OK with the release.

[mjmahone]

Release: https://github.com/graphql/graphql-js/releases/tag/v14.0.0

Please let me know if there's something I'm missing in the release notes. I made sure to go through all the merged PRs, but it's possible I missed something important.

[leebyron]

Thanks to all your hard work, both of you! Let’s keep our eyes open for any issues that might require a patch release and work to ensure all our first class projects can peer-dep on v14

[leebyron]

I’d suggest adding a “depreciations” section to the notes to specifically highlight what has been deprecated in v14 ahead of v15

[IvanGoncharov]

@leebyron Thanks Lee

@mjmahone It also missing #1284 and migration strategy: https://github.com/graphql/graphql-js/pull/1284#issuecomment-377811537

[mjmahone]

Added deprecations based on index.js, and gave migration instructions as described in #1284.

[mjmahone]

That was one of the easiest major releases to upgrade in my living memory: there were flow errors anywhere there were problems, and it was immediately clear what the issue actually was. Some of the ease may be due to having fixed a lot of super-clowniness earlier when I was testing the "rc" release.

As a summary of things we ran into internally at FB with the upgrade:

• A few queries were of the form query Foo($requiredVariable: ID = null), where the usage of $requiredVariable is as a non-nullable argument. This is now a validation error! 🙌
• Had to change flow types that were TypeSystemDefinitionNode to TypeSystemDefinitionNode | TypeSystemExtensionNode, because we split extensions from definitions. These functions probably deserve an audit, and some of them should be split.
• Relay had to do strict-object-typing of a Location type.
• A few places we check for definition.name, we had to switch if (node.kind === SCHEMA_DEFINITION) to if (node.kind === SCHEMA_DEFINITION || node.kind === SCHEMA_EXTENSION) to cover the two type system definitions/extensions that have no name field.
• A few places I had to explicitly give a const someDefinition: OperationDefinitionNode = ... instead of having it be implicit.

[leebyron]

That’s great to hear! For your last point, I’d think AST nodes would be the easiest to type implicitly since they have the “kind” field, making them a disjoint union. No big deal to add an explicit type, but that seems like a warning light to me that something else is wrong

[IvanGoncharov]

@mjmahone I agree with Lee it either problem with our typings or Flow.
Is it part of open source project? If not can you please split it out into separate example so we can together figure out what's the problem?

[mjmahone]

I'm pretty sure it's sketchy code we're doing internally that no one should copy. It was really easy to fix.

It's something like:

function thisIsAProblem(): DocumentNode {
  const someOperationAST: OperationDefinitionNode = {
    kind: OPERATION_DEFINITION,
    ...
  };
  return modifiesDocument({
    kind: DOCUMENT,
    definitions: [{
      ...someOperationAST,
      variableDefinitions: createsVariables(),
    }],
  }));
}

function modifiesDocument(doc: DocumentNode): DocumentNode { ... }
function createsVariables(): Array<VariableDefinitionNode> { ... }

Flow can't figure out how to disambiguate this. The error is:

Could not decide which case to select. Since case 1 [1] may work but if it doesn't case 3 [2] looks promising too. To
fix add a type annotation to .directives [3], to .kind [3], to .loc [3], to .name [3], to .operation [3], to
.selectionSet [3], or to .variableDefinitions [3].

However, it's trivially solved by doing:

function thisIsNotAProblem(): DocumentNode {
  const someOperationAST: OperationDefinitionNode = {
    kind: OPERATION_DEFINITION,
    ...
  };
  const modifiedOperation: OperationDefinitionNode = {
    ...someOperationAST,
    variableDefinitions: createsVariables(),
  };
  return modifiesDocument({
    kind: DOCUMENT,
    definitions: [modifiedOperation],
  }));

[IvanGoncharov]

@mjmahone Thanks for sharing
Agree, it's probably too much for a flow since it's hard to figure out exact type after spread and also definitions accept the long list of possible types.

[IvanGoncharov]

Also OperationDefinitionNode is not exact type and it probably makes it harder to figure out the type of spread:
https://github.com/graphql/graphql-js/blob/6eec989f89a81bf81b8f0cdb6ceebf55d6edd93a/src/language/ast.js#L210-L218