Skip to content

Be selective about instanceof #4386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@

// The GraphQL.js version info.
export { version, versionInfo } from './version.js';
export { setEnv } from './utilities/env.js';
export type { Env } from './utilities/env.js';

// The primary entry point into fulfilling a GraphQL request.
export type { GraphQLArgs } from './graphql.js';
Expand Down
8 changes: 7 additions & 1 deletion src/jsutils/__tests__/instanceOf-test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
import { expect } from 'chai';
import { describe, it } from 'mocha';
import { beforeEach, describe, it } from 'mocha';

import { setEnv } from '../../utilities/env.js';

import { instanceOf } from '../instanceOf.js';

describe('instanceOf', () => {
beforeEach(() => {
setEnv('development');
});

it('do not throw on values without prototype', () => {
class Foo {
get [Symbol.toStringTag]() {
Expand Down
68 changes: 28 additions & 40 deletions src/jsutils/instanceOf.ts
Original file line number Diff line number Diff line change
@@ -1,40 +1,28 @@
import { isProduction } from '../utilities/env.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';

/**
* A replacement for instanceof which includes an error warning when multi-realm
* constructors are detected.
* 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 (isProduction()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the old version, we only checked isProduction() once, this one does it on every call. Can we prove there is no performance hit via benchmarks?

Copy link
Member Author

@JoviDeCroock JoviDeCroock May 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, I don't think doing a check for boolean === boolean is going to be that much of a performance hit. The code is inline-cacheable etc. It's also a feature as this can now be toggled on selectively for requests. We're never going to have a way without some global build-time variable to pre-compile a function, this is against what we're trying to achieve.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what i get on my machine:

image

I am not trying to put up any roadblocks, just checking to make sure that we are all aligned on the costs/benefits.

The alternative is something like: #3915 which could be added to v17 and pushed to the latest patch version of v16.

instanceof has been a blocker for a long time, and so I'm not trying to advocate for a particular solution if you feel strongly about the addition of this feature.

return value instanceof constructor;
}

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.

Ensure that there is only one instance of "graphql" in the node_modules
directory. If different versions of "graphql" are the dependencies of other
Expand All @@ -46,11 +34,11 @@ 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;
};
);
}
}
return false;
}

interface Constructor {
prototype: {
Expand Down
9 changes: 9 additions & 0 deletions src/utilities/env.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export type Env = 'production' | 'development';

let env: Env = 'production';

export const setEnv = (newEnv: Env): void => {
env = newEnv;
};

export const isProduction = (): boolean => env === 'production';
Loading