Skip to content
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
5 changes: 4 additions & 1 deletion .lintstagedrc.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
{
"*.{js,ts}": ["eslint --max-warnings 0 --fix", "prettier --write"],
"*.{js,ts}": [
"eslint --max-warnings 0 --fix --no-warn-ignored",
"prettier --write"
],
"*.json": "prettier --write",
"*.md": "prettier --write"
}
3 changes: 1 addition & 2 deletions eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ export default tsConfig(
'npmDist',
'npmEsmDist',
'denoDist',
'website/.next',
'website/out',
'website/',
'integrationTests/ts/*.ts',
],
},
Expand Down
4 changes: 2 additions & 2 deletions src/jsutils/__tests__/instanceOf-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,10 @@ describe('instanceOf', () => {
const Foo2 = getFoo();

expect(() => instanceOf(new Foo1(), Foo2)).to.throw(
/^Cannot use Foo "{}" from another module or realm./m,
/Cannot use Foo from another module or realm/m,
);
expect(() => instanceOf(new Foo2(), Foo1)).to.throw(
/^Cannot use Foo "{}" from another module or realm./m,
/Cannot use Foo from another module or realm/m,
);
});
});
66 changes: 19 additions & 47 deletions src/jsutils/instanceOf.ts
Original file line number Diff line number Diff line change
@@ -1,56 +1,28 @@
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.

Ensure that there is only one instance of "graphql" in the node_modules
directory. If different versions of "graphql" are the dependencies of other
relied on modules, use "resolutions" to ensure only one version is installed.

https://yarnpkg.com/en/docs/selective-version-resolutions

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 instanceOf(value: unknown, constructor: Constructor): boolean {
if (value instanceof constructor) {
return true;
}
if (typeof value === 'object' && value !== null) {
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) {
throw new Error(
`Multiple GraphQL instances detected, Cannot use ${className} from another module or realm. Read more at https://graphql-js.org/errors/conflicting-versions`,
);
}
}
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a really hot path, are we sure we want to add more logic to it in production?

Copy link
Member Author

@JoviDeCroock JoviDeCroock Feb 16, 2025

Choose a reason for hiding this comment

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

The benchmarks showed no signs of serious regressions

found 0 vulnerabilities
⏱   Recreate a GraphQLSchema
  2 tests completed.

  local x 1,053 ops/sec ±1.82% x 453 KB/op (13 runs sampled)
  HEAD  x 1,064 ops/sec ±0.23% x 453 KB/op (12 runs sampled)

⏱   Build Schema from AST
  2 tests completed.

  local x 173 ops/sec ±0.31% x 3.75 MB/op (9 runs sampled)
  HEAD  x 172 ops/sec ±0.20% x 3.75 MB/op (11 runs sampled)

⏱   Build Schema from Introspection
  2 tests completed.

  local x 183 ops/sec ±0.20% x 3.84 MB/op (11 runs sampled)
  HEAD  x 183 ops/sec ±0.47% x 3.84 MB/op (12 runs sampled)

⏱   Execute Introspection Query
  2 tests completed.

  local x 62.99 ops/sec ±2.58% x 30.31 MB/op (5 runs sampled)
  HEAD  x 63.56 ops/sec ±0.33% x 30.31 MB/op (5 runs sampled)

⏱   Execute Asynchronous List Field
  2 tests completed.

  local x 2,996 ops/sec ±0.37% x 925 KB/op (31 runs sampled)
  HEAD  x 2,981 ops/sec ±0.53% x 925 KB/op (31 runs sampled)

⏱   Execute Async Iterable List Field
  2 tests completed.

  local x 2,566 ops/sec ±0.90% x 686 KB/op (30 runs sampled)
  HEAD  x 2,561 ops/sec ±0.79% x 687 KB/op (31 runs sampled)

⏱   Execute Synchronous List Field
  2 tests completed.

  local x 5,941 ops/sec ±0.63% x 188 KB/op (33 runs sampled)
  HEAD  x 5,926 ops/sec ±0.78% x 188 KB/op (31 runs sampled)

⏱   Parse introspection query
  2 tests completed.

  local x 16,738 ops/sec ±0.13% x 55.93 KB/op (16 runs sampled)
  HEAD  x 16,747 ops/sec ±0.21% x 55.93 KB/op (18 runs sampled)

⏱   Many repeated fields
  2 tests completed.

  local x 88.62 ops/sec ±0.79% x 6.01 MB/op (9 runs sampled)
  HEAD  x 88.57 ops/sec ±1.01% x 6.06 MB/op (8 runs sampled)

⏱   Validate Introspection Query
  2 tests completed.

  local x 1,128 ops/sec ±1.48% x 417 KB/op (9 runs sampled)
  HEAD  x 1,131 ops/sec ±1.57% x 417 KB/op (5 runs sampled)

⏱   Validate Invalid Query
  2 tests completed.

  local x 861 ops/sec ±0.17% x 553 KB/op (8 runs sampled)
  HEAD  x 860 ops/sec ±0.14% x 553 KB/op (10 runs sampled)

⏱   Validate SDL Document
  2 tests completed.

  local x 122 ops/sec ±0.75% x 3.34 MB/op (5 runs sampled)
  HEAD  x 122 ops/sec ±0.14% x 3.34 MB/op (5 runs sampled)

⏱   Visit all AST nodes
  2 tests completed.

  local x 416 ops/sec ±0.84% x 2.07 MB/op (16 runs sampled)
  HEAD  x 417 ops/sec ±0.32% x 2.07 MB/op (16 runs sampled)

⏱   Visit all AST nodes in parallel
  2 tests completed.

  local x  43.7 ops/sec ±0.76% x 2.21 MB/op (5 runs sampled)
  HEAD  x 43.97 ops/sec ±0.37% x 2.21 MB/op (5 runs sampled)

Copy link
Member Author

@JoviDeCroock JoviDeCroock Feb 16, 2025

Choose a reason for hiding this comment

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

The alternative is that we completely scrap instanceof and actually make multiple versions of graphql work alongside each other. The process.env check has caused so much grievances.

My main goal is to move v17 to beta/release-candidate versions

Copy link
Member

Choose a reason for hiding this comment

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

actually make multiple versions of graphql work alongside each other

I, personally, think this would be a mistake. What if one supports oneof and the other doesn't? Or stream/defer? Fragment arguments?

The benchmarks showed no signs of serious regressions

Great; glad we found my concerns unwarranted :) Carry on :)

Copy link
Contributor

Choose a reason for hiding this comment

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

On my machine, I get a performance regression with this change:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Running the benchmark github action...


interface Constructor {
prototype: {
Expand Down
1 change: 1 addition & 0 deletions website/pages/_meta.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const meta = {
title: 'FAQ',
},
'going-to-production': '',
errors: '',
'api-v16': {
type: 'menu',
title: 'API',
Expand Down
12 changes: 12 additions & 0 deletions website/pages/errors/conflicting-versions.mdx
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Conflicting GraphQL versions

Ensure that there is only one instance of "graphql" in the node_modules
directory. If different versions of "graphql" are the dependencies of other
relied on modules, use "resolutions" to ensure only one version is installed.

https://yarnpkg.com/en/docs/selective-version-resolutions

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.
Loading