Graphql-js: Redesign type/schema API to remove instanceof checks

[Pajn]

Per the discussion in #377

The current behavior makes it impossible to use graphql with npm links which is a huge problem if you have multiple interdependent packages as you'll have to rerun npm install for every single change and waste a lot of time.

[stubailo]

Looks like this is because of a limitation in Flow?

[taion]

BTW, this is probably not an unfamiliar limitation, but I believe you can work around this by using symlinks in the linked packages.

[Pajn]

By limitation in Flow, do you mean that it's something that first needs to be fixed on their side before the instance of checks can be removed? Should I file an issue?
I don't know Flow (we uses TS instead) but I would expect that there is a way to assert the type without affecting non-flow users.

Thanks for the tip about symlinking graphql, that certainly helps!

[josephsavona]

We've run into this a bunch during Relay development as well. I think the answer might be to fix npm though.

[leebyron]

Yeah - unfortunately we currently rely on instanceof both for how graphql works, but also because of how Flow works.

I don't think it's crazy to change this in the near future, but that would be a very real breaking change for all those dependent on the current types API.

[glasser]

We ran into an issue where we got:

Error: Can only create NonNull of a Nullable GraphQLType but got: ID. 
at invariant (node_modules/graphql/jsutils/invariant.js:19:11) 
at new GraphQLNonNull (node_modules/graphql/type/definition.js:651:29) 

ie, it was telling us that we weren't allowed to say ID!. This turned out to be multiple loaded versions of graphql and failing instanceof checks. Is that what this issue is about?

If so, maybe one partial solution would be to make graphql detect multiple loaded versions and print a warning? I believe React does that.

[jquense]

Hi ya'll, This is really annoying issue. Would everyone be open to leveraging Symbol.hasInstance to maybe address this? It should work for newer versions of node without breaking anything. e.g

class GraphQLObjectType {
  static [Symbol.hasInstance](foo) {
    return foo && foo[Symbol.for('GraphQLObjectType')];
  }
}

GraphQLObjectType.prototype[Symbol.for('GraphQLObjectType')] = true;

Should Allow the instanceof checks to continue to work without relying on the default JS behavior for them

[felixfbecker]

This has bitten me multiple times now:

• https://github.com/avantcredit/gql2ts/issues/85
• https://github.com/graphcool/graphql-cli/issues/23
  the worst part about this was that the assertion here has no error message, it just fails with an empty string.

It's almost impossible to _not_ end up with multiple graphql versions when using a lot of graphql tooling, because graphqljs is still 0.x, so it is not under semver and npm treats the difference between 0.15 and 0.16 as a breaking version, resulting in package duplication if one package depends on ^0.16 and one on ^0.15.

instanceof is really a bad practice in a language that uses ducktyping and node module resolution. You should never care about what class instance an object is, you should only care about whether it provides the properties you need.

[amannn]

If somebody is having trouble with this, I just learned that yarn supports forcing all dependencies to resolve to the same version. You can add this to your package.json:

"resolutions": {
  "graphql": "0.11.7"
}

If somebody runs into errors like Schema must be an instance of GraphQLSchema. Also ensure that there are not multiple versions of GraphQL installed in your node_modules directory. this can be a helpful workaround.

[leebyron]

Great suggestion @amannn - that's exactly what we do at Facebook for this module and a few others where having a single version is important.

[felixfbecker]

While that is a viable workaround for yarn users imo this shouldn't be required. Packages should state what versions they are compatible with and npm/yarn should be able to install dependencies, installing multiple versions where needed, and everything should work as long as version requirements are satisfied. Imo the user shouldn't have to hack the dependency graph (essentially overpowering defined dependency ranges) to make this work

[jquense]

COUGH #989 ;)

[taion]

This was resolved in v0.12 and should be closed

[jquense]

hmm it is technically resolved but doesn't really solve the issue. It still uses instanceof checks but moves it to a utility function. The issue is still that this will fail if you have two instances of graphql installed

[leebyron]

After gathering more insight on this issue over the recent months, I think it's time to close this issue.

The problem here is that instanceof check is really a sort of "canary in the coal mine". Including multiple copies of graphql-js in a bundle is almost never what you want. It doubles the byte-weight of the library in your bundle. Even if instanceof was replaced with a duplicate-safe brand-check, allowing different versions of the library to interact with each other is almost guaranteed to cause other serious issues that will be much harder to debug.

Overall, avoiding duplicate bytes in your bundle and avoiding hard to debug issues is more important than making npm link easier to use. More important is to provide better information sooner so the "bite" of this problem is less arduous.

Breaking the problem down a bit, we've made serious improvements in the most recent versions of the library. First, predicate utility functions were introduced so that no user of GraphQL.js needs to use instanceof themselves. This not only offers better API consistency with existing predicate functions and makes code easier to read, it also unifies the actual instance check. That allowed for a much better error message to be produced which is not only more descriptive about the larger issue and how to fix it, but is also much more likely to be triggered in the case that it should, as opposed to previous versions which would simply return false for the instanceof check and break in harder to understand ways.

[jquense]

my main problem with this is that there is no way to solve this without using poorly documented feature in yarn, and there is no recourse withnpm. On top of that almost _everyone_ that consumesgraphql-js` does so as a direct dependency, this would be less of a problem if people at least declared it a peer dep.

Overall, avoiding duplicate bytes in your bundle and avoiding hard to debug issues is more important than making npm link easier to use.

I don't understand this rationale. Why do care about the byte size of my http server or tool. I don't care about npm link either? It's frustratingly difficult to build abstractions on top of graphql-js because of this. Gatsby, doesn't depend on or ask anyone to use npm link, but anyone using gatsby suffers from this issue constantly because both gatsby and and relay depend on graphql-js and we are beholden to the whims of weather yarn or npm decide to dedupe it

[felixfbecker]

Yeah I don't really understand that argument either. This is mostly about GraphQL _tooling_, which you have as devDependencies, and I don't care at all about how many KB I am downloading for that. I am not bundling them.

I see the point of not wanting to allow different arbitrary versions of graphqljs to work with each other. But currently, it's not possible to even have multiple installations of the _same_ version work with each other, and that is the problem. Say you have two GraphQL tools installed - get-graphql-schema and gql2ts-cli (the same could be gatsby and relay). Let's say get-graphql-schema depends on graphql@^0.11, so node_modules looks like this:

|- lodash
|- get-graphql-schema
'- [email protected]

Now gql2ts-cli depends on two packages, gql2ts-from-query and gql2ts-util. These two each depend on graphql@^0.12, so now node_modules looks like this:

|- lodash
|- gql2ts-cli
|- gql2ts-from-schema
|  '- graphql (0.12)
|- gql2ts-util
|  '- graphql (0.12)
|- get-graphql-schema
'- graphql (0.11)

The package manager acted 100% correctly: It delivered every package its declared dependency of graphql, resolvable by the node module resolution algorithm. Every contract was satisfied, so this setup should work. But it doesn't, because gql2ts-from-schema indirectly does an instanceof check on a class instance that came from gql2ts-util. Even though it is the exact same version, that will always fail.

Did the devs of the project or the packages do something wrong here? No. The fact that get-graphql-schema depends on 0.11 shouldn't matter, it is only a CLI to download a schema, and it does so just fine and fulfils its contract. gql2ts-cli also declares its dependencies correctly and its subpackages each fulfil their contract.

The only workaround I know with npm is to install gql2ts-cli with the --global-style flag and persist the structure to a package-lock or shrinkwrap file, which would basically circumvent deduplication completely for that package:

|- lodash (4.0.0)
|- gql2ts-cli
|  |- gql2ts-from-schema
|  |- gql2ts-util
|  |- lodash (4.0.0)
|  '- graphql (0.12)
|- get-graphql-schema
'- graphql (0.11)

Which is obviously bad because now every common shared dependency (in this example, lodash) is not deduplicated anymore. Such a workaround should not be needed, it's incredibly hard for a newcomer to understand what and why he/she has to do this, which harms the growth of graphql in the community.

The only other option is to use yarn's resolutions field or for every package to make graphql to a peerDependency - which it is not. It's not "plugging in" to graphql like a Gulp plugin, Angular directive or an express middleware. It's exposing its own API and uses (imports) graphql under the hood. With peerDependencies we would get into the problem of as soon as there is an API breaking change in a major release in graphql somewhere in that dependency tree that is now required by one tool, all the other tools would not be working anymore until all tools are upgraded to work with a common version again.
E.g. even though get-graphql-schema's job is only to download a schema file and it could still do so perfectly fine with its outdated [email protected], it now cannot anymore because we are forcing every package to work with the same version (either because package's peerDependency ranges need to be compatible or because of yarn's resolutions field).
That is against every benefit of how local package management and module resolution in NodeJS works - it brings us the version conflict hell that we have in other language's package managers that can only handle a single global version of any package.

[chrbala]

I've had the most luck with using a monorepo setup and yarn's resolutions field. I've had no issues with this configuration.

For non-monorepos that I needed to yarn link together, what worked best for me (for dev, of course) was globally installing graphql and yarn linking graphql to each package that required graphql. This would probably not work when a different version of graphql is a transitive dependency, however.

I generally disagree with the decision here, but at least for my preferred use case (monorepo), the solution is not too bad – as long as the library never releases backward-incompatible changes that force tooling libs to be updated before graphql is updated (due to the version override).

[felixfbecker]

as long as the library never releases backward-incompatible changes that force tooling libs to be updated before graphql is updated (due to the version override).

Well the last three releases (0.10, 0.11, 0.12) with breaking changes where in May, August and December, so on average a breaking release every few months. If you use yarn's resolution field then that puts a significant burden on package maintainers to always stay compatible with the latest version, because you will only be able to upgrade once _all_ your packages work with the new version. You are basically overpowering their declared version ranges.

[AdamZaczek]

I got this issue today, even though I have not been updating packages. I tripple checked all the commits and nothing in package.json/yarn.lock has been changed in a while.
All I did was running my expo app from my coworkers phone once(he uses windows for now and simply doesnt commit yarn.locks). After that I could no longer run my app, even on simulator.
Adding resolutions does not change anything for me, neither does removing node_modules or clearing yarn cache. Do you guys have any ideas what might be the solution?