From 213b7034e35ad80d273efc5235981513f39b85e1 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 26 Jan 2021 11:39:07 -0500 Subject: [PATCH 1/2] Avoid relying on constructor.name for instanceOf error check. An improvement beyond #1174, for non-production environments that enable minification, such as NODE_ENV='staging'. Since graphql-js goes to the trouble of providing a Symbol.toStringTag property for most of the classes it exports, and that string is immune to minification (unlike constructor.name), we should use it for error checking in instanceOf(value, constructor) whenever the constructor provides a tag, falling back to constructor.name and value.constructor.name for constructors that do not define Symbol.toStringTag (as before). Motivating issue/investigation: https://github.com/apollographql/apollo-client/issues/7446#issuecomment-767660631 --- src/jsutils/__tests__/instanceOf-test.js | 111 +++++++++++++++++++++++ src/jsutils/instanceOf.js | 23 ++++- 2 files changed, 131 insertions(+), 3 deletions(-) diff --git a/src/jsutils/__tests__/instanceOf-test.js b/src/jsutils/__tests__/instanceOf-test.js index dfa364c9ce..2fa4fb39fd 100644 --- a/src/jsutils/__tests__/instanceOf-test.js +++ b/src/jsutils/__tests__/instanceOf-test.js @@ -2,6 +2,15 @@ import { expect } from 'chai'; import { describe, it } from 'mocha'; import { instanceOf } from '../instanceOf'; +import { SYMBOL_TO_STRING_TAG } from '../../polyfills/symbols'; +import { + GraphQLScalarType, + GraphQLObjectType, + GraphQLInterfaceType, + GraphQLUnionType, + GraphQLEnumType, + GraphQLInputObjectType, +} from '../../type/definition'; describe('instanceOf', () => { it('fails with descriptive error message', () => { @@ -19,4 +28,106 @@ describe('instanceOf', () => { /^Cannot use Foo "\[object Object\]" from another module or realm./m, ); }); + + describe('Symbol.toStringTag', () => { + function checkSameNameClasses(getClass) { + const Class1 = getClass('FirstClass'); + const Class2 = getClass('SecondClass'); + + expect(Class1.name).to.equal('Foo'); + expect(Class2.name).to.equal('Foo'); + + expect(instanceOf(null, Class1)).to.equal(false); + expect(instanceOf(null, Class2)).to.equal(false); + + const c1 = new Class1(); + const c2 = new Class2(); + + expect(getTag(c1)).to.equal('FirstClass'); + expect(getTag(c2)).to.equal('SecondClass'); + + // In these Symbol.toStringTag tests, instanceOf returns the + // expected boolean value without throwing an error, because even + // though Class1.name === Class2.name, the Symbol.toStringTag + // strings of the two classes are different. + expect(instanceOf(c1, Class1)).to.equal(true); + expect(instanceOf(c1, Class2)).to.equal(false); + expect(instanceOf(c2, Class1)).to.equal(false); + expect(instanceOf(c2, Class2)).to.equal(true); + } + + function getTag(from: any): string { + return from[SYMBOL_TO_STRING_TAG]; + } + + it('does not fail if dynamically-defined tags differ', () => { + checkSameNameClasses((tag) => { + class Foo {} + Object.defineProperty(Foo.prototype, SYMBOL_TO_STRING_TAG, { + value: tag, + }); + return Foo; + }); + }); + + it('does not fail if dynamically-defined tag getters differ', () => { + checkSameNameClasses((tag) => { + class Foo {} + Object.defineProperty(Foo.prototype, SYMBOL_TO_STRING_TAG, { + get() { + return tag; + }, + }); + return Foo; + }); + }); + + it('does not fail for anonymous classes', () => { + checkSameNameClasses((tag) => { + const Foo = class {}; + Object.defineProperty(Foo.prototype, SYMBOL_TO_STRING_TAG, { + get() { + return tag; + }, + }); + return Foo; + }); + }); + + it('does not fail if prototype property tags differ', () => { + checkSameNameClasses((tag) => { + class Foo {} + (Foo.prototype: any)[SYMBOL_TO_STRING_TAG] = tag; + return Foo; + }); + }); + + it('does not fail if computed getter tags differ', () => { + checkSameNameClasses((tag) => { + class Foo { + // $FlowFixMe[unsupported-syntax] Flow doesn't support computed properties yet + get [SYMBOL_TO_STRING_TAG]() { + return tag; + } + } + return Foo; + }); + }); + + it('is defined for various GraphQL*Type classes', () => { + function checkGraphQLType(constructor, expectedName) { + expect(getTag(constructor.prototype)).to.equal(expectedName); + const instance = Object.create(constructor.prototype); + expect(getTag(instance)).to.equal(expectedName); + expect(instanceOf(instance, constructor)).to.equal(true); + } + + checkGraphQLType(GraphQLScalarType, 'GraphQLScalarType'); + checkGraphQLType(GraphQLObjectType, 'GraphQLObjectType'); + checkGraphQLType(GraphQLInterfaceType, 'GraphQLInterfaceType'); + checkGraphQLType(GraphQLUnionType, 'GraphQLUnionType'); + checkGraphQLType(GraphQLEnumType, 'GraphQLEnumType'); + checkGraphQLType(GraphQLInputObjectType, 'GraphQLInputObjectType'); + }); + }); }); diff --git a/src/jsutils/instanceOf.js b/src/jsutils/instanceOf.js index bdecd4cdfb..638c483c65 100644 --- a/src/jsutils/instanceOf.js +++ b/src/jsutils/instanceOf.js @@ -1,3 +1,5 @@ +import { SYMBOL_TO_STRING_TAG } from '../polyfills/symbols'; + /** * A replacement for instanceof which includes an error warning when multi-realm * constructors are detected. @@ -15,9 +17,24 @@ export const instanceOf: (mixed, mixed) => boolean = return true; } if (value) { - const valueClass = value.constructor; - const className = constructor.name; - if (className && valueClass && valueClass.name === className) { + const proto = constructor && constructor.prototype; + const classTag = proto && proto[SYMBOL_TO_STRING_TAG]; + const className = classTag || constructor.name; + // When the constructor class defines a Symbol.toStringTag + // property, as most classes exported by graphql-js do, use it + // instead of constructor.name and value.constructor.name to + // detect module/realm duplication, since the Symbol.toStringTag + // string is immune to minification. This code runs only when + // process.env.NODE_ENV !== 'production', but minification is + // often enabled in non-production environments like 'staging'. + // In these environments, this error can be thrown mistakenly if + // we rely on constructor.name and value.constructor.name, since + // they could be minified to the same short string, even though + // value is legitimately _not_ instanceof constructor. + const valueName = classTag + ? value[SYMBOL_TO_STRING_TAG] + : value.constructor && value.constructor.name; + if (typeof className === 'string' && valueName === className) { throw new Error( `Cannot use ${className} "${value}" from another module or realm. From 717f24edeb019eade967fef39bd786689b7a4f7c Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 25 Mar 2021 11:55:25 -0400 Subject: [PATCH 2/2] Stop using SYMBOL_TO_STRING_TAG polyfill. After rebasing PR #2894 against origin/main, I noticed that the src/polyfills/symbols.js module no longer exists, and Symbol.toStringTag is used directly elsewhere in the codebase. --- src/jsutils/__tests__/instanceOf-test.js | 13 ++++++------- src/jsutils/instanceOf.js | 8 +++----- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/jsutils/__tests__/instanceOf-test.js b/src/jsutils/__tests__/instanceOf-test.js index 2fa4fb39fd..3739587c5f 100644 --- a/src/jsutils/__tests__/instanceOf-test.js +++ b/src/jsutils/__tests__/instanceOf-test.js @@ -2,7 +2,6 @@ import { expect } from 'chai'; import { describe, it } from 'mocha'; import { instanceOf } from '../instanceOf'; -import { SYMBOL_TO_STRING_TAG } from '../../polyfills/symbols'; import { GraphQLScalarType, GraphQLObjectType, @@ -57,13 +56,13 @@ describe('instanceOf', () => { } function getTag(from: any): string { - return from[SYMBOL_TO_STRING_TAG]; + return from[Symbol.toStringTag]; } it('does not fail if dynamically-defined tags differ', () => { checkSameNameClasses((tag) => { class Foo {} - Object.defineProperty(Foo.prototype, SYMBOL_TO_STRING_TAG, { + Object.defineProperty(Foo.prototype, Symbol.toStringTag, { value: tag, }); return Foo; @@ -73,7 +72,7 @@ describe('instanceOf', () => { it('does not fail if dynamically-defined tag getters differ', () => { checkSameNameClasses((tag) => { class Foo {} - Object.defineProperty(Foo.prototype, SYMBOL_TO_STRING_TAG, { + Object.defineProperty(Foo.prototype, Symbol.toStringTag, { get() { return tag; }, @@ -85,7 +84,7 @@ describe('instanceOf', () => { it('does not fail for anonymous classes', () => { checkSameNameClasses((tag) => { const Foo = class {}; - Object.defineProperty(Foo.prototype, SYMBOL_TO_STRING_TAG, { + Object.defineProperty(Foo.prototype, Symbol.toStringTag, { get() { return tag; }, @@ -97,7 +96,7 @@ describe('instanceOf', () => { it('does not fail if prototype property tags differ', () => { checkSameNameClasses((tag) => { class Foo {} - (Foo.prototype: any)[SYMBOL_TO_STRING_TAG] = tag; + (Foo.prototype: any)[Symbol.toStringTag] = tag; return Foo; }); }); @@ -106,7 +105,7 @@ describe('instanceOf', () => { checkSameNameClasses((tag) => { class Foo { // $FlowFixMe[unsupported-syntax] Flow doesn't support computed properties yet - get [SYMBOL_TO_STRING_TAG]() { + get [Symbol.toStringTag]() { return tag; } } diff --git a/src/jsutils/instanceOf.js b/src/jsutils/instanceOf.js index 638c483c65..02b62999fd 100644 --- a/src/jsutils/instanceOf.js +++ b/src/jsutils/instanceOf.js @@ -1,5 +1,3 @@ -import { SYMBOL_TO_STRING_TAG } from '../polyfills/symbols'; - /** * A replacement for instanceof which includes an error warning when multi-realm * constructors are detected. @@ -18,7 +16,7 @@ export const instanceOf: (mixed, mixed) => boolean = } if (value) { const proto = constructor && constructor.prototype; - const classTag = proto && proto[SYMBOL_TO_STRING_TAG]; + const classTag = proto && proto[Symbol.toStringTag]; const className = classTag || constructor.name; // When the constructor class defines a Symbol.toStringTag // property, as most classes exported by graphql-js do, use it @@ -32,8 +30,8 @@ export const instanceOf: (mixed, mixed) => boolean = // they could be minified to the same short string, even though // value is legitimately _not_ instanceof constructor. const valueName = classTag - ? value[SYMBOL_TO_STRING_TAG] - : value.constructor && value.constructor.name; + ? value[Symbol.toStringTag] + : value.constructor && value.constructor.name; if (typeof className === 'string' && valueName === className) { throw new Error( `Cannot use ${className} "${value}" from another module or realm.