From d60813ebaf5aaaaa092008ccb1840876854a2589 Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Wed, 4 Jun 2025 12:39:06 +0300 Subject: [PATCH 1/3] Use no-op for production instanceOf Re-implements #4386 with a configurable function variable initialized to a no-op function in production builds. This is apparently optimizable by v8, as it has no performance hit when setEnv('development') is not called. This should also allow for tree-shaking in production builds, as the development instanceOf function should not be included within the production bundle if setEnv is not called, although this has not been confirmed. To avoid a performance hit, setEnv('development') calls setInstanceOfCheckForEnv('development') which sets the function variable to developmentInstanceOfCheck. Setting a property on globalThis was tested, but reading the property led to a small performance hit. Co-Authored-By: Yaacov Rydzinski --- src/index.ts | 2 + src/jsutils/__tests__/instanceOf-test.ts | 55 +++++++++++++----- src/jsutils/instanceOf.ts | 72 +++++++++++++----------- src/setEnv.ts | 7 +++ 4 files changed, 90 insertions(+), 46 deletions(-) create mode 100644 src/setEnv.ts diff --git a/src/index.ts b/src/index.ts index 7bba9d8eee..e2ac15f444 100644 --- a/src/index.ts +++ b/src/index.ts @@ -28,6 +28,8 @@ // The GraphQL.js version info. export { version, versionInfo } from './version.js'; +export { setEnv } from './setEnv.js'; +export type { Env } from './setEnv.js'; // The primary entry point into fulfilling a GraphQL request. export type { GraphQLArgs } from './graphql.js'; diff --git a/src/jsutils/__tests__/instanceOf-test.ts b/src/jsutils/__tests__/instanceOf-test.ts index 5a54a641e5..498946dced 100644 --- a/src/jsutils/__tests__/instanceOf-test.ts +++ b/src/jsutils/__tests__/instanceOf-test.ts @@ -1,9 +1,25 @@ import { expect } from 'chai'; import { describe, it } from 'mocha'; +import { setEnv } from '../../setEnv.js'; + import { instanceOf } from '../instanceOf.js'; describe('instanceOf', () => { + function oldVersion() { + class Foo {} + return Foo; + } + + function newVersion() { + class Foo { + get [Symbol.toStringTag]() { + return 'Foo'; + } + } + return Foo; + } + it('do not throw on values without prototype', () => { class Foo { get [Symbol.toStringTag]() { @@ -16,28 +32,39 @@ describe('instanceOf', () => { expect(instanceOf(Object.create(null), Foo)).to.equal(false); }); - it('detect name clashes with older versions of this lib', () => { - function oldVersion() { - class Foo {} - return Foo; - } + it('ignore name clashes with older versions of this lib by default', () => { + const NewClass = newVersion(); + const OldClass = oldVersion(); + expect(instanceOf(new NewClass(), NewClass)).to.equal(true); + expect(instanceOf(new OldClass(), NewClass)).to.equal(false); + }); - function newVersion() { - class Foo { - get [Symbol.toStringTag]() { - return 'Foo'; - } - } - return Foo; - } + it('detect name clashes with older versions of this lib when set', () => { + setEnv('development'); + + const NewClass = newVersion(); + const OldClass = oldVersion(); + expect(instanceOf(new NewClass(), NewClass)).to.equal(true); + expect(() => instanceOf(new OldClass(), NewClass)).to.throw(); + }); + + it('ignore name clashes with older versions of this lib when reset', () => { + setEnv('development'); const NewClass = newVersion(); const OldClass = oldVersion(); expect(instanceOf(new NewClass(), NewClass)).to.equal(true); expect(() => instanceOf(new OldClass(), NewClass)).to.throw(); + + setEnv('production'); + + expect(instanceOf(new NewClass(), NewClass)).to.equal(true); + expect(instanceOf(new OldClass(), NewClass)).to.equal(false); }); it('allows instances to have share the same constructor name', () => { + setEnv('development'); + function getMinifiedClass(tag: string) { class SomeNameAfterMinification { get [Symbol.toStringTag]() { @@ -58,6 +85,8 @@ describe('instanceOf', () => { }); it('fails with descriptive error message', () => { + setEnv('development'); + function getFoo() { class Foo { get [Symbol.toStringTag]() { diff --git a/src/jsutils/instanceOf.ts b/src/jsutils/instanceOf.ts index 66811433ae..f0e155fac1 100644 --- a/src/jsutils/instanceOf.ts +++ b/src/jsutils/instanceOf.ts @@ -1,10 +1,12 @@ +import type { Env } from '../setEnv.js'; + import { inspect } from './inspect.js'; -/* c8 ignore next 3 */ -const isProduction = - globalThis.process != null && - // eslint-disable-next-line no-undef - process.env.NODE_ENV === 'production'; +const noOp = (_value: unknown, _constructor: Constructor): void => { + /* no-op */ +}; + +let check = noOp; /** * A replacement for instanceof which includes an error warning when multi-realm @@ -12,29 +14,30 @@ const isProduction = * See: https://expressjs.com/en/advanced/best-practice-performance.html#set-node_env-to-production * See: https://webpack.js.org/guides/production/ */ -export const instanceOf: (value: unknown, constructor: Constructor) => boolean = - /* c8 ignore next 6 */ - // FIXME: https://github.com/graphql/graphql-js/issues/2317 - isProduction - ? function instanceOf(value: unknown, constructor: Constructor): boolean { - return value instanceof constructor; - } - : function instanceOf(value: unknown, constructor: Constructor): boolean { - if (value instanceof constructor) { - return true; - } - if (typeof value === 'object' && value !== null) { - // Prefer Symbol.toStringTag since it is immune to minification. - const className = constructor.prototype[Symbol.toStringTag]; - const valueClassName = - // We still need to support constructor's name to detect conflicts with older versions of this library. - Symbol.toStringTag in value - ? value[Symbol.toStringTag] - : value.constructor?.name; - if (className === valueClassName) { - const stringifiedValue = inspect(value); - throw new Error( - `Cannot use ${className} "${stringifiedValue}" from another module or realm. +export function instanceOf(value: unknown, constructor: Constructor): boolean { + if (value instanceof constructor) { + return true; + } + check(value, constructor); + return false; +} + +function developmentInstanceOfCheck( + value: unknown, + constructor: Constructor, +): void { + if (typeof value === 'object' && value !== null) { + // Prefer Symbol.toStringTag since it is immune to minification. + const className = constructor.prototype[Symbol.toStringTag]; + const valueClassName = + // We still need to support constructor's name to detect conflicts with older versions of this library. + Symbol.toStringTag in value + ? value[Symbol.toStringTag] + : value.constructor?.name; + if (className === valueClassName) { + const stringifiedValue = inspect(value); + throw new Error( + `Cannot use ${className} "${stringifiedValue}" from another module or realm. Ensure that there is only one instance of "graphql" in the node_modules directory. If different versions of "graphql" are the dependencies of other @@ -46,11 +49,14 @@ Duplicate "graphql" modules cannot be used at the same time since different versions may have different capabilities and behavior. The data from one version used in the function from another could produce confusing and spurious results.`, - ); - } - } - return false; - }; + ); + } + } +} + +export function setInstanceOfCheckForEnv(newEnv: Env): void { + check = newEnv === 'development' ? developmentInstanceOfCheck : noOp; +} interface Constructor { prototype: { diff --git a/src/setEnv.ts b/src/setEnv.ts new file mode 100644 index 0000000000..390df06f97 --- /dev/null +++ b/src/setEnv.ts @@ -0,0 +1,7 @@ +import { setInstanceOfCheckForEnv } from './jsutils/instanceOf.js'; + +export type Env = 'production' | 'development'; + +export function setEnv(newEnv: Env): void { + setInstanceOfCheckForEnv(newEnv); +} From d18a8d934e1c54052d08746da94437db2689e15f Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Fri, 6 Jun 2025 12:00:24 +0300 Subject: [PATCH 2/3] add ability to check env --- src/__tests__/env-test.ts | 20 ++++++++++++++++++++ src/index.ts | 2 +- src/setEnv.ts | 7 +++++++ 3 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 src/__tests__/env-test.ts diff --git a/src/__tests__/env-test.ts b/src/__tests__/env-test.ts new file mode 100644 index 0000000000..cfc27eaee3 --- /dev/null +++ b/src/__tests__/env-test.ts @@ -0,0 +1,20 @@ +import { expect } from 'chai'; +import { describe, it } from 'mocha'; + +import { getEnv, setEnv } from '../setEnv.js'; + +describe('Env', () => { + it('should return undefined if environment is not set', () => { + expect(getEnv()).to.equal(undefined); + }); + + it('should set the environment to development', () => { + setEnv('development'); + expect(getEnv()).to.equal('development'); + }); + + it('should set the environment to production', () => { + setEnv('production'); + expect(getEnv()).to.equal('production'); + }); +}); diff --git a/src/index.ts b/src/index.ts index e2ac15f444..885bd13417 100644 --- a/src/index.ts +++ b/src/index.ts @@ -28,7 +28,7 @@ // The GraphQL.js version info. export { version, versionInfo } from './version.js'; -export { setEnv } from './setEnv.js'; +export { setEnv, getEnv } from './setEnv.js'; export type { Env } from './setEnv.js'; // The primary entry point into fulfilling a GraphQL request. diff --git a/src/setEnv.ts b/src/setEnv.ts index 390df06f97..e4277b0792 100644 --- a/src/setEnv.ts +++ b/src/setEnv.ts @@ -2,6 +2,13 @@ import { setInstanceOfCheckForEnv } from './jsutils/instanceOf.js'; export type Env = 'production' | 'development'; +let env: Env | undefined; + export function setEnv(newEnv: Env): void { + env = newEnv; setInstanceOfCheckForEnv(newEnv); } + +export function getEnv(): Env | undefined { + return env; +} From c2549cfb857ec2376d2a3abe9b0fac8c81eb79cf Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Mon, 9 Jun 2025 12:55:23 +0300 Subject: [PATCH 3/3] do not allow env to be reset --- src/__tests__/env-test.ts | 26 ++++++-- src/env.ts | 21 +++++++ src/index.ts | 4 +- src/jsutils/__tests__/instanceOf-test.ts | 75 ++++++++++++------------ src/jsutils/instanceOf.ts | 10 ++-- src/setEnv.ts | 14 ----- 6 files changed, 86 insertions(+), 64 deletions(-) create mode 100644 src/env.ts delete mode 100644 src/setEnv.ts diff --git a/src/__tests__/env-test.ts b/src/__tests__/env-test.ts index cfc27eaee3..4432c56c78 100644 --- a/src/__tests__/env-test.ts +++ b/src/__tests__/env-test.ts @@ -1,20 +1,34 @@ import { expect } from 'chai'; import { describe, it } from 'mocha'; -import { getEnv, setEnv } from '../setEnv.js'; +const { getEnv: defaultGetEnv } = await import(`../env.js?ts${Date.now()}`); +const { getEnv: developmentGetEnv, setEnv: developmentSetEnv } = await import( + `../env.js?ts${Date.now()}` +); +const { getEnv: productionGetEnv, setEnv: productionSetEnv } = await import( + `../env.js?ts${Date.now()}` +); +const { setEnv: repetitiveSetEnv } = await import(`../env.js?ts${Date.now()}`); describe('Env', () => { it('should return undefined if environment is not set', () => { - expect(getEnv()).to.equal(undefined); + expect(defaultGetEnv()).to.equal(undefined); }); it('should set the environment to development', () => { - setEnv('development'); - expect(getEnv()).to.equal('development'); + developmentSetEnv('development'); + expect(developmentGetEnv()).to.equal('development'); }); it('should set the environment to production', () => { - setEnv('production'); - expect(getEnv()).to.equal('production'); + productionSetEnv('production'); + expect(productionGetEnv()).to.equal('production'); + }); + + it('should throw if environment already set', () => { + repetitiveSetEnv('development'); + expect(() => repetitiveSetEnv('production')).to.throw( + 'Environment already set to "development", cannot be changed to "production".', + ); }); }); diff --git a/src/env.ts b/src/env.ts new file mode 100644 index 0000000000..7db3bbbc36 --- /dev/null +++ b/src/env.ts @@ -0,0 +1,21 @@ +import { useDevelopmentInstanceOfCheck } from './jsutils/instanceOf.js'; + +export type Env = 'production' | 'development'; + +let env: Env | undefined; + +export function setEnv(newEnv: Env): void { + if (env !== undefined && env !== newEnv) { + throw new Error( + `Environment already set to "${env}", cannot be changed to "${newEnv}".`, + ); + } + env = newEnv; + if (env === 'development') { + useDevelopmentInstanceOfCheck(); + } +} + +export function getEnv(): Env | undefined { + return env; +} diff --git a/src/index.ts b/src/index.ts index 885bd13417..942f3e4636 100644 --- a/src/index.ts +++ b/src/index.ts @@ -28,8 +28,8 @@ // The GraphQL.js version info. export { version, versionInfo } from './version.js'; -export { setEnv, getEnv } from './setEnv.js'; -export type { Env } from './setEnv.js'; +export { setEnv, getEnv } from './env.js'; +export type { Env } from './env.js'; // The primary entry point into fulfilling a GraphQL request. export type { GraphQLArgs } from './graphql.js'; diff --git a/src/jsutils/__tests__/instanceOf-test.ts b/src/jsutils/__tests__/instanceOf-test.ts index 498946dced..965abc0cbb 100644 --- a/src/jsutils/__tests__/instanceOf-test.ts +++ b/src/jsutils/__tests__/instanceOf-test.ts @@ -1,24 +1,19 @@ import { expect } from 'chai'; -import { describe, it } from 'mocha'; +import { before, describe, it } from 'mocha'; -import { setEnv } from '../../setEnv.js'; +import { setEnv } from '../../env.js'; import { instanceOf } from '../instanceOf.js'; -describe('instanceOf', () => { - function oldVersion() { - class Foo {} - return Foo; - } +// will pick up non-development version of instanceOf by default +const { instanceOf: defaultInstanceOf } = await import( + `../instanceOf.js?ts${Date.now()}` +); - function newVersion() { - class Foo { - get [Symbol.toStringTag]() { - return 'Foo'; - } - } - return Foo; - } +describe('instanceOf', () => { + before(() => { + setEnv('development'); + }); it('do not throw on values without prototype', () => { class Foo { @@ -32,15 +27,20 @@ describe('instanceOf', () => { expect(instanceOf(Object.create(null), Foo)).to.equal(false); }); - it('ignore name clashes with older versions of this lib by default', () => { - const NewClass = newVersion(); - const OldClass = oldVersion(); - expect(instanceOf(new NewClass(), NewClass)).to.equal(true); - expect(instanceOf(new OldClass(), NewClass)).to.equal(false); - }); - it('detect name clashes with older versions of this lib when set', () => { - setEnv('development'); + function oldVersion() { + class Foo {} + return Foo; + } + + function newVersion() { + class Foo { + get [Symbol.toStringTag]() { + return 'Foo'; + } + } + return Foo; + } const NewClass = newVersion(); const OldClass = oldVersion(); @@ -48,23 +48,28 @@ describe('instanceOf', () => { expect(() => instanceOf(new OldClass(), NewClass)).to.throw(); }); - it('ignore name clashes with older versions of this lib when reset', () => { - setEnv('development'); + it('ignore name clashes with older versions of this lib by default', () => { + function oldVersion() { + class Foo {} + return Foo; + } + + function newVersion() { + class Foo { + get [Symbol.toStringTag]() { + return 'Foo'; + } + } + return Foo; + } const NewClass = newVersion(); const OldClass = oldVersion(); - expect(instanceOf(new NewClass(), NewClass)).to.equal(true); - expect(() => instanceOf(new OldClass(), NewClass)).to.throw(); - - setEnv('production'); - - expect(instanceOf(new NewClass(), NewClass)).to.equal(true); - expect(instanceOf(new OldClass(), NewClass)).to.equal(false); + expect(defaultInstanceOf(new NewClass(), NewClass)).to.equal(true); + expect(defaultInstanceOf(new OldClass(), NewClass)).to.equal(false); }); it('allows instances to have share the same constructor name', () => { - setEnv('development'); - function getMinifiedClass(tag: string) { class SomeNameAfterMinification { get [Symbol.toStringTag]() { @@ -85,8 +90,6 @@ describe('instanceOf', () => { }); it('fails with descriptive error message', () => { - setEnv('development'); - function getFoo() { class Foo { get [Symbol.toStringTag]() { diff --git a/src/jsutils/instanceOf.ts b/src/jsutils/instanceOf.ts index f0e155fac1..b036c0f8e3 100644 --- a/src/jsutils/instanceOf.ts +++ b/src/jsutils/instanceOf.ts @@ -1,5 +1,3 @@ -import type { Env } from '../setEnv.js'; - import { inspect } from './inspect.js'; const noOp = (_value: unknown, _constructor: Constructor): void => { @@ -8,6 +6,10 @@ const noOp = (_value: unknown, _constructor: Constructor): void => { let check = noOp; +export function useDevelopmentInstanceOfCheck(): void { + check = developmentInstanceOfCheck; +} + /** * A replacement for instanceof which includes an error warning when multi-realm * constructors are detected. @@ -54,10 +56,6 @@ spurious results.`, } } -export function setInstanceOfCheckForEnv(newEnv: Env): void { - check = newEnv === 'development' ? developmentInstanceOfCheck : noOp; -} - interface Constructor { prototype: { [Symbol.toStringTag]: string; diff --git a/src/setEnv.ts b/src/setEnv.ts deleted file mode 100644 index e4277b0792..0000000000 --- a/src/setEnv.ts +++ /dev/null @@ -1,14 +0,0 @@ -import { setInstanceOfCheckForEnv } from './jsutils/instanceOf.js'; - -export type Env = 'production' | 'development'; - -let env: Env | undefined; - -export function setEnv(newEnv: Env): void { - env = newEnv; - setInstanceOfCheckForEnv(newEnv); -} - -export function getEnv(): Env | undefined { - return env; -}