Feedback on v16.0.0-rc.1 #3245

IvanGoncharov · 2021-08-26T11:10:36Z

Can you please test your lib/project against https://github.com/graphql/graphql-js/releases/tag/v16.0.0-rc.1 and leave feedback on this issue.

kamilkisiela · 2021-08-26T12:49:27Z

@apollo/federation:0.30.0 and previous versions

Cannot find module 'graphql/validation/rules/UniqueTypeNames'
at @apollo/federation/dist/composition/rules.js

kamilkisiela · 2021-08-26T12:53:08Z

Property 'isDeprecated' does not exist on type 'GraphQLField<any, any, { [argument: string]: any; }>'

Same story for Enums and other types.

kamilkisiela · 2021-08-26T12:55:50Z

ts(2724): '"graphql"' has no exported member named 'Visitor'. Did you mean 'visit'?

kamilkisiela · 2021-08-26T12:58:49Z

resolveType no longer accepts a type

  Type 'GraphQLObjectType<any, any>' is not assignable to type 'PromiseOrValue<string | undefined>'.
    Type 'GraphQLObjectType<any, any>' is not assignable to type 'string'.

I guess it previously was supported because of toString() and TS allowed for that, now it's not.

PabloSzx · 2021-08-26T18:15:53Z

Could collectFields be re-exported from /execution/execute.ts? this will help @graphql-tools/utils to not break with graphql 16, see:

https://github.com/ardatan/graphql-tools/blob/master/packages/utils/src/visitResult.ts#L19

EDIT: Created PR #3246

PabloSzx · 2021-08-26T18:22:42Z

Another thing, can a experimental-stream-defer version of this RC also be released?

yaacovCR · 2021-08-26T20:00:21Z

Could collectFields be re-exported from /execution/execute.ts? this will help @graphql-tools/utils to not break with graphql 16, see:

fixed/refactored on main #3187

Could be backported

PabloSzx · 2021-08-26T20:11:39Z

Could collectFields be re-exported from /execution/execute.ts? this will help @graphql-tools/utils to not break with graphql 16, see:

fixed/refactored on main #3187

Could be backported

#3246

saihaj · 2021-08-26T20:30:20Z

Another thing, can a experimental-stream-defer version of this RC also be released?

Yeah we need this @IvanGoncharov. I was looking into graphiql and there defer and stream is used

saihaj · 2021-08-27T00:37:28Z

`express-graphql`

Updating works fine all the tests pass. Only issue is that there peer deps conflicts which requires updating graphiql and subscriptions-transport-ws so once a new version for them is released we can update it

`graphql-relay-js`

There are expected breaking changes that require some changes. Since we no longer export flow types so we should do this upgrade part of TS migration.

n1ru4l · 2021-08-27T06:44:28Z

Breaks 'apollo-server-core':

const apolloServer = new ApolloServerBase({ schema, debug });

yields in the exception

      Must provide document.

      at devAssert (node_modules/graphql/jsutils/devAssert.js:12:11)
      at assertValidExecutionArguments (node_modules/graphql/execution/execute.js:145:40)
      at Object.execute (node_modules/graphql/execution/execute.js:74:3)
      at Object.generateSchemaHash (node_modules/apollo-server-core/src/utils/schemaHash.ts:12:18)
      at ApolloServerBase.generateSchemaDerivedData (node_modules/apollo-server-core/src/ApolloServer.ts:671:24)
      at Object.schemaDerivedDataProvider (node_modules/apollo-server-core/src/ApolloServer.ts:288:18)
      at new SchemaManager (node_modules/apollo-server-core/src/utils/schemaManager.ts:75:15)
      at new ApolloServerBase (node_modules/apollo-server-core/src/ApolloServer.ts:283:24)

Seems like apollo-server is using the legacy execute call signature execute(schema, ...args) instead of execute({ schema, ...args })

n1ru4l · 2021-08-27T08:18:30Z

Previously the mapAsyncIterator function could be imported via

import mapAsyncIterator from 'graphql/subscription/mapAsyncIterator.js';

but the export was changed to import { mapAsyncIterator } from 'graphql/subscription/mapAsyncIterator.js';

Could we still have a default export for having easy backward compatibility without having to inline the function into our libraries?

I created #3247 for this simple change

n1ru4l · 2021-08-27T08:51:21Z

Parser class attributes are private and should be protected instead for implementing custom parsers. #3248

n1ru4l · 2021-08-27T11:53:43Z

GraphQLError now has this:

https://github.com/n1ru4l/graphql-js/blob/4493ca3d1281e01635570824f70867aa68610323/src/error/GraphQLError.ts#L204-L207

But the comment indicates it should be removed? SHould this be removed or is the comment outdated? This also causes TS errors for apollo-server.

n1ru4l · 2021-08-27T12:24:16Z

The breaking change for resolveType is unnecessary. We can have the signature

export type GraphQLTypeResolver<TSource, TContext> = (
  value: TSource,
  context: TContext,
  info: GraphQLResolveInfo,
  abstractType: GraphQLAbstractType,
) => PromiseOrValue<GraphQLObjectType | string | undefined>;

and then in the GraphQL.js 17 release change it to

export type GraphQLTypeResolver<TSource, TContext> = (
  value: TSource,
  context: TContext,
  info: GraphQLResolveInfo,
  abstractType: GraphQLAbstractType,
) => PromiseOrValue<string | undefined>;

That gives people and tooling around GraphQL.js more flexibility.

I created #3249 for this.

ardatan · 2021-08-27T12:47:30Z

In < v15 there was visitorKeys parameter that allows us to visit only specified nodes;

visit(node, visitors, {
  SelectionSet: ['selections'],
  Field: ['selectionSet'],
  InlineFragment: ['selectionSet'],
  FragmentDefinition: ['selectionSet'],
})

But now there is no third parameter for visitorKeys. But this parameter was important for the runtime performance of Schema Stitching. We use visitorKeys to avoid visitor to visit unwanted nodes. Since we heavily use visit multiple times with different visitorKeys combinations, this is really important for the performance of Schema Stitching. I'd create a PR but unfortunately I couldn't find where this change was made because history is a bit confusing due to Flow->TS migration.

About all of the issues written here so far, I think it would be better not to release v16 until fixing these issues because those are heavily from the popular packages.
I'd be appreciated if my comment is noticed otherwise this kind of major release will lead much more issues because of this kind of breaking changes.

For this kind of breaking changes I'd suggest to keep backwards compatibility with a deprecation notice in the first major(maybe minor) release to warn people for upcoming breaking change then remove it completely on the following major.

IvanGoncharov · 2021-08-27T13:19:53Z

@n1ru4l Thanks for your PRs to apollo-server.
I'm also working on the same fixes in parallel for Apollo server/client & federation.

Cannot find module 'graphql/validation/rules/UniqueTypeNames'
at @apollo/federation/dist/composition/rules.js

@kamilkisiela I will fix that in the federation repo.

Property 'isDeprecated' does not exist on type 'GraphQLField<any, any, { [argument: string]: any; }>'

@kamilkisiela Yes, it was first deprecated and then removed:

graphql-js/src/type/definition.js

Lines 1016 to 1017 in d2d8458

    
           // @deprecated and will be removed in v16 
        
           isDeprecated: boolean,

You should use deprecationReason instead and it is also supported in v15.x.x

ts(2724): '"graphql"' has no exported member named 'Visitor'. Did you mean 'visit'?

It was renamed to ASTVisitor to match other names like ASTNode

saihaj · 2021-08-27T13:21:16Z

@ardatan #2930 this PR removed visitorKeys we can try to revert this and convert to TS

ardatan · 2021-08-27T13:26:08Z

@IvanGoncharov
I think we can export Visitor as an alias with a deprecation notice for now in order to keep backwards compatibility, no?
@saihaj Thank you. I'll create a PR soon.

IvanGoncharov · 2021-09-20T16:31:48Z

I think we can export Visitor as an alias with a deprecation notice for now in order to keep backwards compatibility, no?

@ardatan ASTVisitor was added more than 2 years ago and is present in both v14 and v15, see:
9366d6d#diff-559cd403e97ad511680f66b39577aff06d8b4e71349b7dc8a4732a31a43ddf75R9

IvanGoncharov · 2021-10-05T19:02:33Z

Released a https://github.com/graphql/graphql-js/releases/tag/v16.0.0-rc.3 with all fixes included, big thanks for all PRs 👍
Please feel free to report here or open a separate issue if you find some problem with RC3.
Also, I released v15.6.1 with some stuff backported from main, see: https://github.com/graphql/graphql-js/releases

IvanGoncharov · 2021-10-08T16:01:13Z

I stuck with updating Apollo Server.
There are some conflicts between ApolloError and changes in theGraphQLError.
Was planning to fix it in graphql-js today (have a partial fix ready) but sadly was distracted by another community issue.
Will try to fix it over the weekend.

yaacovCR · 2021-10-14T08:45:18Z

#3316

IvanGoncharov · 2021-10-23T12:43:44Z

I figured migration path for GraphQLError from v15 to v16.
Just released RC6 and plan release 16.0.0 on Monday evening.
Not a lot of changes since previous releases but would be great if you can test it.

PabloSzx · 2021-10-23T16:53:05Z

I feel like graphql/graphiql#1982 should be fixed before releasing v16

IvanGoncharov · 2021-10-23T18:58:58Z

@PabloSzx I left a comment on how to fix it.
If you have time please test with GraphiQL.
As I'm forced by The Guild to release v16 before the next WG I don't have time to test it on GraphiQL.
So the release is happing on Monday unless anything major happens.

saihaj · 2021-10-27T00:39:27Z

To test GraphiQL we need to publish RC version that has defer-and-stream support.

IvanGoncharov · 2021-10-27T12:18:40Z

@saihaj Current plan:

I will cut one more RC right now
@robrichard Will rebase stream-defer on top of that RC
I release RC with stream/defer
We can test it on chore: support graphql-js@16 graphiql#1983
Release 16.0.0 🎉

IvanGoncharov · 2021-10-27T16:20:24Z

@saihaj published https://github.com/graphql/graphql-js/releases/tag/v16.0.0-rc.7.experimental-stream-defer.4

IvanGoncharov · 2021-10-27T18:28:57Z

As discussed on WG we are ready to cut release.
One thing it's better to do in the morning is to be on the safe side.
I will cut it first thing in the morning (around 7 AM UTC).

IvanGoncharov · 2021-10-28T15:02:51Z

Release script is broke due to funny error:

It turns out we never tested src/version.ts on release without preReleaseTag.
Working on a fix will release in the short while.

IvanGoncharov · 2021-10-28T20:10:35Z

Released https://github.com/graphql/graphql-js/releases/tag/v16.0.0 🎉

saihaj · 2021-10-28T20:17:09Z

#3346

IvanGoncharov · 2021-10-28T20:20:54Z

Will release defer-stream tomorrow

IvanGoncharov · 2021-10-29T09:20:18Z

Released https://github.com/graphql/graphql-js/releases/tag/v16.0.0-experimental-stream-defer.5

IvanGoncharov pinned this issue Aug 26, 2021

n1ru4l mentioned this issue Aug 27, 2021

fix: add getter for field Symbol.toStringTag whose absence causes Typ… apollographql/apollo-server#5663

Closed

ardatan mentioned this issue Aug 27, 2021

Bring visitorKeys back #3250

Merged

saihaj closed this as completed Oct 28, 2021

IvanGoncharov unpinned this issue Oct 28, 2021

mcrawshaw mentioned this issue Nov 3, 2021

Update to support GraphQL JS v16 contra/graphql-helix#87

Closed

Feedback on v16.0.0-rc.1 #3245

Feedback on v16.0.0-rc.1 #3245

Comments

IvanGoncharov commented Aug 26, 2021

kamilkisiela commented Aug 26, 2021

kamilkisiela commented Aug 26, 2021

kamilkisiela commented Aug 26, 2021

kamilkisiela commented Aug 26, 2021

PabloSzx commented Aug 26, 2021 • edited Loading

PabloSzx commented Aug 26, 2021

yaacovCR commented Aug 26, 2021

PabloSzx commented Aug 26, 2021

saihaj commented Aug 26, 2021

saihaj commented Aug 27, 2021

express-graphql

graphql-relay-js

n1ru4l commented Aug 27, 2021

n1ru4l commented Aug 27, 2021

n1ru4l commented Aug 27, 2021

n1ru4l commented Aug 27, 2021 • edited Loading

n1ru4l commented Aug 27, 2021 • edited Loading

ardatan commented Aug 27, 2021 • edited Loading

IvanGoncharov commented Aug 27, 2021 • edited Loading

saihaj commented Aug 27, 2021

ardatan commented Aug 27, 2021

IvanGoncharov commented Sep 20, 2021

IvanGoncharov commented Oct 5, 2021

IvanGoncharov commented Oct 8, 2021

yaacovCR commented Oct 14, 2021

IvanGoncharov commented Oct 23, 2021

PabloSzx commented Oct 23, 2021

IvanGoncharov commented Oct 23, 2021

saihaj commented Oct 27, 2021

IvanGoncharov commented Oct 27, 2021

IvanGoncharov commented Oct 27, 2021

IvanGoncharov commented Oct 27, 2021

IvanGoncharov commented Oct 28, 2021

IvanGoncharov commented Oct 28, 2021

saihaj commented Oct 28, 2021

IvanGoncharov commented Oct 28, 2021

IvanGoncharov commented Oct 29, 2021 • edited Loading

PabloSzx commented Aug 26, 2021 •

edited

Loading

`express-graphql`

`graphql-relay-js`

n1ru4l commented Aug 27, 2021 •

edited

Loading

n1ru4l commented Aug 27, 2021 •

edited

Loading

ardatan commented Aug 27, 2021 •

edited

Loading

IvanGoncharov commented Aug 27, 2021 •

edited

Loading

IvanGoncharov commented Oct 29, 2021 •

edited

Loading