Skip to content

Commit 02d302e

Browse files
authored
predicates: replace instanceof with symbol based brand check (#4468)
This PR updates our predicates to rely on a brand check rather than `instanceof`. We still pass brand check to our `instanceOf` utility and in development mode use the same logic as previously to throw our error with multiple instances of `graphql`.
1 parent 51b9794 commit 02d302e

File tree

6 files changed

+125
-44
lines changed

6 files changed

+125
-44
lines changed

src/jsutils/__tests__/instanceOf-test.ts

Lines changed: 33 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,17 @@ import { instanceOf } from '../instanceOf.js';
55

66
describe('instanceOf', () => {
77
it('do not throw on values without prototype', () => {
8+
const fooSymbol: unique symbol = Symbol('Foo');
89
class Foo {
10+
readonly __kind: symbol = fooSymbol;
911
get [Symbol.toStringTag]() {
1012
return 'Foo';
1113
}
1214
}
1315

14-
expect(instanceOf(true, Foo)).to.equal(false);
15-
expect(instanceOf(null, Foo)).to.equal(false);
16-
expect(instanceOf(Object.create(null), Foo)).to.equal(false);
16+
expect(instanceOf(true, fooSymbol, Foo)).to.equal(false);
17+
expect(instanceOf(null, fooSymbol, Foo)).to.equal(false);
18+
expect(instanceOf(Object.create(null), fooSymbol, Foo)).to.equal(false);
1719
});
1820

1921
it('detect name clashes with older versions of this lib', () => {
@@ -23,56 +25,66 @@ describe('instanceOf', () => {
2325
}
2426

2527
function newVersion() {
26-
class Foo {
28+
const fooSymbol: unique symbol = Symbol('Foo');
29+
class FooClass {
30+
readonly __kind: symbol = fooSymbol;
2731
get [Symbol.toStringTag]() {
2832
return 'Foo';
2933
}
3034
}
31-
return Foo;
35+
return { fooSymbol, FooClass };
3236
}
3337

34-
const NewClass = newVersion();
38+
const { fooSymbol: newSymbol, FooClass: NewClass } = newVersion();
3539
const OldClass = oldVersion();
36-
expect(instanceOf(new NewClass(), NewClass)).to.equal(true);
37-
expect(() => instanceOf(new OldClass(), NewClass)).to.throw();
40+
expect(instanceOf(new NewClass(), newSymbol, NewClass)).to.equal(true);
41+
expect(() => instanceOf(new OldClass(), newSymbol, NewClass)).to.throw();
3842
});
3943

4044
it('allows instances to have share the same constructor name', () => {
4145
function getMinifiedClass(tag: string) {
46+
const someSymbol: unique symbol = Symbol(tag);
4247
class SomeNameAfterMinification {
48+
readonly __kind: symbol = someSymbol;
4349
get [Symbol.toStringTag]() {
4450
return tag;
4551
}
4652
}
47-
return SomeNameAfterMinification;
53+
return { someSymbol, SomeNameAfterMinification };
4854
}
4955

50-
const Foo = getMinifiedClass('Foo');
51-
const Bar = getMinifiedClass('Bar');
52-
expect(instanceOf(new Foo(), Bar)).to.equal(false);
53-
expect(instanceOf(new Bar(), Foo)).to.equal(false);
56+
const { someSymbol: fooSymbol, SomeNameAfterMinification: Foo } =
57+
getMinifiedClass('Foo');
58+
const { someSymbol: barSymbol, SomeNameAfterMinification: Bar } =
59+
getMinifiedClass('Bar');
60+
expect(instanceOf(new Foo(), barSymbol, Bar)).to.equal(false);
61+
expect(instanceOf(new Bar(), fooSymbol, Foo)).to.equal(false);
5462

55-
const DuplicateOfFoo = getMinifiedClass('Foo');
56-
expect(() => instanceOf(new DuplicateOfFoo(), Foo)).to.throw();
57-
expect(() => instanceOf(new Foo(), DuplicateOfFoo)).to.throw();
63+
const {
64+
someSymbol: duplicateOfFooSymbol,
65+
SomeNameAfterMinification: DuplicateOfFoo,
66+
} = getMinifiedClass('Foo');
67+
expect(() => instanceOf(new DuplicateOfFoo(), fooSymbol, Foo)).to.throw();
68+
expect(() => instanceOf(new Foo(), duplicateOfFooSymbol, Foo)).to.throw();
5869
});
5970

6071
it('fails with descriptive error message', () => {
6172
function getFoo() {
73+
const fooSymbol: unique symbol = Symbol('Foo');
6274
class Foo {
6375
get [Symbol.toStringTag]() {
6476
return 'Foo';
6577
}
6678
}
67-
return Foo;
79+
return { fooSymbol, Foo };
6880
}
69-
const Foo1 = getFoo();
70-
const Foo2 = getFoo();
81+
const { fooSymbol: foo1Symbol, Foo: Foo1 } = getFoo();
82+
const { fooSymbol: foo2Symbol, Foo: Foo2 } = getFoo();
7183

72-
expect(() => instanceOf(new Foo1(), Foo2)).to.throw(
84+
expect(() => instanceOf(new Foo1(), foo2Symbol, Foo2)).to.throw(
7385
/^Cannot use Foo "{}" from another module or realm./m,
7486
);
75-
expect(() => instanceOf(new Foo2(), Foo1)).to.throw(
87+
expect(() => instanceOf(new Foo2(), foo1Symbol, Foo1)).to.throw(
7688
/^Cannot use Foo "{}" from another module or realm./m,
7789
);
7890
});

src/jsutils/instanceOf.ts

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,20 +7,29 @@ const isProduction =
77
process.env.NODE_ENV === 'production';
88

99
/**
10-
* A replacement for instanceof which includes an error warning when multi-realm
11-
* constructors are detected.
10+
* A replacement for instanceof relying on a symbol-driven type brand which in
11+
* development mode includes an error warning when multi-realm constructors are
12+
* detected.
1213
* See: https://expressjs.com/en/advanced/best-practice-performance.html#set-node_env-to-production
1314
* See: https://webpack.js.org/guides/production/
1415
*/
15-
export const instanceOf: (value: unknown, constructor: Constructor) => boolean =
16-
/* c8 ignore next 6 */
16+
export const instanceOf: (
17+
value: unknown,
18+
symbol: symbol,
19+
constructor: Constructor,
20+
) => boolean =
21+
/* c8 ignore next 9 */
1722
// FIXME: https://github.com/graphql/graphql-js/issues/2317
1823
isProduction
19-
? function instanceOf(value: unknown, constructor: Constructor): boolean {
20-
return value instanceof constructor;
24+
? function instanceOf(value: unknown, symbol: symbol): boolean {
25+
return (value as any)?.__kind === symbol;
2126
}
22-
: function instanceOf(value: unknown, constructor: Constructor): boolean {
23-
if (value instanceof constructor) {
27+
: function instanceOf(
28+
value: unknown,
29+
symbol: symbol,
30+
constructor: Constructor,
31+
): boolean {
32+
if ((value as any)?.__kind === symbol) {
2433
return true;
2534
}
2635
if (typeof value === 'object' && value !== null) {

src/language/source.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ interface Location {
66
column: number;
77
}
88

9+
const sourceSymbol: unique symbol = Symbol('Source');
10+
911
/**
1012
* A representation of source input to GraphQL. The `name` and `locationOffset` parameters are
1113
* optional, but they are useful for clients who store GraphQL documents in source files.
@@ -14,6 +16,8 @@ interface Location {
1416
* The `line` and `column` properties in `locationOffset` are 1-indexed.
1517
*/
1618
export class Source {
19+
readonly __kind: symbol;
20+
1721
body: string;
1822
name: string;
1923
locationOffset: Location;
@@ -23,6 +27,7 @@ export class Source {
2327
name: string = 'GraphQL request',
2428
locationOffset: Location = { line: 1, column: 1 },
2529
) {
30+
this.__kind = sourceSymbol;
2631
this.body = body;
2732
this.name = name;
2833
this.locationOffset = locationOffset;
@@ -47,5 +52,5 @@ export class Source {
4752
* @internal
4853
*/
4954
export function isSource(source: unknown): source is Source {
50-
return instanceOf(source, Source);
55+
return instanceOf(source, sourceSymbol, Source);
5156
}

0 commit comments

Comments
 (0)