Apollo-client: graphql-client throws Error instance instead of ApolloError instance.

[lucasconstantino]

I'm experiencing a slight breaking change from 0.5.10 to 0.5.26. Basically, errors thrown from _graphql-client_ are instances of ApolloError at the first, but not at the later.

Here goes a code to reproduce this issue:

import React, { Component } from 'react'
import { ApolloError } from 'apollo-client/errors/ApolloError'
import { graphql } from 'react-apollo'
import gql from 'graphql-tag'

class MyContainer extends Component {
  save () {
    this.props.mutate()
      .catch(error => {
        if (error instanceof ApolloError) {
          // This works in 0.5.10
          // This does not work in 0.5.26
          alert('Error is an instance of ApolloError')
        }
      })
  }

  render () {
    return (
      <button onClick={ this.save }>Save</button>
    )
  }
}

const query = gql`
  mutation MutationName
    mutate {
      resultingField
    }
  }
`

export default graphql(query)(MyContainer)

I found this problem doing error normalization using the previous version, but once I've update the module, the code stopped working.

[calebmer]

Is this behavior still in 0.7.3? I’m not sure if we have the capacity to support older versions. If you have a legitimate reason that keeps you in 0.5.x then please let us know!

[lucasconstantino]

@calebmer I'll try to update to 0.7.3. I do understand the lack of support right now, and as far as my understanding of SemVer goes I think it's ok to have eventual breaking changes while a major version is not released. It is a strange issue though. If I happen to have the same issue with version 0.7.3 I'll report back here and either continue using 0.5.10 or work on a pull-request.

Thanks for your feedback.

[lucasconstantino]

Hello! I've upgraded to 0.7.3 and there still persists. It's interesting to note two things about this version:

1. The class ApolloError is now exported as high-level api (which means I can import { ApolloError } from 'apollo-client');
2. Apollo internally seams to assert if an error is an Apollo error by simply verifying the existence of the graphQLErrors property in the error object. But this seams to be handled differently on the ts version of the files. I don't really understand how TypeScript compiles, but can we suppose this might be an issue with TypeScript itself?

Here goes the two files I'm talking about, in the TypeScript and JavaScript version, as the second is not available at this repository but only after compiling locally:

./errors/ApolloError.d.ts:

/// <reference types="graphql" />
import { GraphQLError } from 'graphql';
export declare function isApolloError(err: Error): err is ApolloError;
export declare class ApolloError extends Error {
    message: string;
    graphQLErrors: GraphQLError[];
    networkError: Error;
    extraInfo: any;
    constructor({graphQLErrors, networkError, errorMessage, extraInfo}: {
        graphQLErrors?: GraphQLError[];
        networkError?: Error;
        errorMessage?: string;
        extraInfo?: any;
    });
}

./errors/ApolloError.js:

var __extends = (this && this.__extends) || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
export function isApolloError(err) {
    return err.hasOwnProperty('graphQLErrors');
}
var generateErrorMessage = function (err) {
    var message = '';
    if (Array.isArray(err.graphQLErrors) && err.graphQLErrors.length !== 0) {
        err.graphQLErrors.forEach(function (graphQLError) {
            message += 'GraphQL error: ' + graphQLError.message + '\n';
        });
    }
    if (err.networkError) {
        message += 'Network error: ' + err.networkError.message + '\n';
    }
    message = message.replace(/\n$/, '');
    return message;
};
var ApolloError = (function (_super) {
    __extends(ApolloError, _super);
    function ApolloError(_a) {
        var graphQLErrors = _a.graphQLErrors, networkError = _a.networkError, errorMessage = _a.errorMessage, extraInfo = _a.extraInfo;
        var _this = _super.call(this, errorMessage) || this;
        _this.graphQLErrors = graphQLErrors;
        _this.networkError = networkError;
        _this.stack = new Error().stack;
        if (!errorMessage) {
            _this.message = generateErrorMessage(_this);
        }
        else {
            _this.message = errorMessage;
        }
        _this.extraInfo = extraInfo;
        return _this;
    }
    return ApolloError;
}(Error));
export { ApolloError };
//# sourceMappingURL=ApolloError.js.map

[calebmer]

I just did some research and what’s happening here is kind of interesting. This is not a bug in TypeScript, ApolloErrors will never be an instance of ApolloError given how Errors work and how JavaScript classes work.

As you can see in the TypeScript output (and the Babel output) if the constructor function (_super) returns a value then that value will be used as this and then eventually returned. Most constructors do not return values:

function Person (name) {
  this.name = name;
}

assert(typeof Person.call({}, 'Caleb') === 'undefined');

However when calling Error a value is returned:

assert(typeof Error.call({}, 'My error message') !== 'undefined');

This means on the line TypeScript builds:

var _this = _super.call(this, errorMessage) || this;

…the default of this is not used because _super is returning a value. This is not something we can stop as that is just how the ECMAScript class implementation works. See Babel for the same behavior. We could, of course, implement ApolloError using the old-fashioned way of defining JavaScript “classes,” a function; or you could use the isApolloError function which it appears like we use internally (or even run the check inside yourself: err.hasOwnProperty('graphQLErrors')). Let me know what your preferred solution is.

So this does appear to be a breaking change in the way TypeScript emits classes. https://unpkg.com/[email protected]/errors/ApolloError.js vs. https://unpkg.com/[email protected]/errors/ApolloError.js. However, it seems like the old TypeScript behavior was incorrect according to the spec.

It looks like @helfer added the duck-typing method isApolloError to workaround this. Now we know why when using the ApolloError constructor we never get actual ApolloError objects 👍

[lucasconstantino]

What a lesson you gave me here :) Thanks a lot for your interest in the matter, really.

I still think it's quite strange that we can't compare types, for I believe this can happen in quite so many other situations other than what I was trying to do. It's odd, indeed.

Well, I guess we can close this issue now :)