Be selective about instanceof #4386
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
selective-production
branch
from
1dbe0d4 to
c97380d
Compare
| throw new Error( | ||
| `Cannot use ${className} "${stringifiedValue}" from another module or realm. | ||
| export function instanceOf(value: unknown, constructor: Constructor): boolean { | ||
| if (isProduction()) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is what i get on my machine:
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.
|
@github-actions run-benchmark |
Looks like GitHub actions have been disabled |
|
Can I make sure I understand the full story here? Is the following correct?
Is that accurate? I'm also a bit confused about what problem we're trying to solve — are people reporting that their use of graphql-js throws the error too often? Not enough? |
We want to enable ergonomic cross-platform use of this package. To provide this specialized error only while "in development" in a cross-platform way, we need to figure out how to drop the dependency on NODE_ENV to signal "in production." As long as we depend on NODE_ENV, then for non-Node environments, a build step is necessary to artificially define NODE_ENV, as detailed in: https://www.graphql-js.org/docs/going-to-production/ Other options:
|
|
So is the goal to completely drop use of NODE_ENV, or just have it not be the only way of setting it? Would it be reasonable to have this PR but using NODE_ENV (if set) for the default value? In that case the change would be backwards-compatible (and we wouldn't even necessarily need to figure it out to unblock v17). |
|
One potential consideration of this approach is that it makes it harder (I think?) to perform dead code elimination in a production build; previously if you set the correct envvar, not only would those code paths be skipped but they could be elided from the bundle entirely. |
|
DCE is the main benefit yes but... with our changes to support browser where we i.e. check globalThis most default configurations won't remove this code path. We had to add the typeof globalThis.process replace for that to work. That being said, if we want to explore other avenues like the global symbol check, my main concern is for this to work with
I'll close this as it looks like the consensus is further exploration. |
Re-implements graphql#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.
Re-implements graphql#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.
Re-implements graphql#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 <yaacovCR@gmail.com
Re-implements graphql#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 <yaacovCR@gmail.com
Re-implements graphql#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 <yaacovCR@gmail.com>
Re-implements graphql#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 <yaacovCR@gmail.com>
Re-implements graphql#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 <yaacovCR@gmail.com>
This creates a new env export that allows the user to selectively opt-in or opt-out of
instanceofchecks and any future debug hints we might introduce