Remove process.env check #4345
Remove process.env check #4345
Conversation
|
Hi @JoviDeCroock, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
JoviDeCroock
force-pushed
the
instance-of-alternative
branch
from
ff49b34 to
bf8ebc5
Compare
JoviDeCroock
added
PR: polish 💅
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I think this is a really hot path, are we sure we want to add more logic to it in production?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
On my machine, I get a performance regression with this change:
There was a problem hiding this comment.
Running the benchmark github action...
src/jsutils/instanceOf.ts
Outdated
| : 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#1`, |
There was a problem hiding this comment.
I recommend that you don't use monotonically increasing error numbers as it will cause conflicts when you have multiple branches being worked on concurrently and introducing new errors. I also recommend that you use a page per error otherwise the singular error page could get overwhelming (and fixing it later may break existing links in deployed applications).
At Graphile we use our "errored" domain for errors, e.g. https://err.red/gev2 - the first character indicates which website it goes to (g for https://grafast.org/errors/) and the ev2 is a code for the error which links to the relevant page under that. This further allows adding sub-anchors such as https://err.red/gev2#stream.
JoviDeCroock
force-pushed
the
instance-of-alternative
branch
from
bf8ebc5 to
e7802b7
Compare
This comment has been minimized.
This comment has been minimized.
Benchmark output
🍳 Preparing HEAD... added 526 packages, and audited 527 packages in 3s 173 packages are looking for funding found 0 vulnerabilities
./npmDist
|
3 participants
This is an alternative approach to removing our
instanceofcheck, when we detect a mismatched version of GraphQL we throw an error regardless of us being in development or production. This is justified because multiple GraphQL version will lead to weird behaviour either way, rather than embedding the whole string we link to our docs where the user can find steps forpnpm, ... we can keep expanding this.