Skip to content
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

Only trigger "one instance of graphql" error in DEV environments. #1174

Merged
merged 1 commit into from
Dec 20, 2017

Conversation

leebyron
Copy link
Contributor

This ensures the additional checking for multiple graphql instances only occurs in non-production builds. Not only does this improve the performance for production builds, it also solves a spurious error case for minified builds:

Since this additional check compares the .name of the constructors as a proxy for determining if two classes are representationally equal, minified builds may result in spurious false-positives if class names are minified to single-character names and those characters are likely to conflict.

This doesn't require process to exist (defaulting to the non-production mode), but bundlers like Webpack should consider using a stripping transform (see: https://webpack.js.org/guides/production/#specify-the-environment) in production builds if they also use a minifier.

This ensures the additional checking for multiple graphql instances only occurs in non-production builds. Not only does this improve the performance for production builds, it also solves a spurious error case for minified builds:

Since this additional check compares the `.name` of the constructors as a proxy for determining if two classes are representationally equal, minified builds may result in spurious false-positives if class names are minified to single-character names and those characters are likely to conflict.

This doesn't require `process` to exist (defaulting to the non-production mode), but bundlers like Webpack should consider using a stripping transform (see: https://webpack.js.org/guides/production/#specify-the-environment) in production builds if they also use a minifier.
@leebyron leebyron force-pushed the dev-only-instanceof branch from 496c1a0 to 9b7a292 Compare December 20, 2017 21:52
@leebyron leebyron merged commit 94234c8 into master Dec 20, 2017
@leebyron leebyron deleted the dev-only-instanceof branch December 20, 2017 22:59
@cyrilgandon
Copy link

That's a big blocker for using new features of graphql-js, any idea when the next release will be? Thank you! 👍

@pleunv
Copy link

pleunv commented Jan 5, 2018

Would love to see this released too as this causes problems that only appear in prod builds.

@danez
Copy link

danez commented Jan 5, 2018

Also waiting for this, graphiql stopped working for us completely in production.

@nparse
Copy link

nparse commented Jan 8, 2018

Struggling with this issue too.

@flip-it
Copy link

flip-it commented Jan 9, 2018

When can we expect this to be released?

@sheerun
Copy link

sheerun commented Jan 10, 2018

Same issue, please release :)

@mabellLU
Copy link

Same issue

@Dermah
Copy link

Dermah commented Jan 11, 2018

FYI for those that desperately need this, we've had success rolling back to graphql@0.11 while waiting for the above fix to be released.

@mabellLU
Copy link

mabellLU commented Jan 11, 2018

Really just need the updated GraphQLErrors object with extensions

@stantoncbradley
Copy link

+1 need GraphQLErrors as well! Thanks!!

@nparse
Copy link

nparse commented Jan 13, 2018

If you guys are having issues with GraphiQL, you can override qraphql dependency in package.json using the most recent version, like so

...
   "dependencies": {
     "graphiql": "^0.11.10",
+    "graphql": "git://github.com/graphql/graphql-js.git#npm",
...

It solved the problem for me.

calocan added a commit to rescapes/helpers that referenced this pull request Jan 23, 2018
@BerndWessels
Copy link

@nparse is this making it into the next release?

Another but maybe related problem is that uglify with mangle=true results in this error:

module initialization error: Error

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.
at n.a (/var/task/index.js:1:156141)
at f (/var/task/index.js:1:3835)
at d (/var/task/index.js:1:3756)
at _ (/var/task/index.js:1:4392)
at n.q (/var/task/index.js:1:2738)
at new o (/var/task/index.js:1:25040)
at o (/var/task/index.js:1:25008)
at Object.n.a.r (/var/task/index.js:1:36681)
at t (/var/task/index.js:1:447)
at Object.<anonymous> (/var/task/index.js:1:83640)

@Cabezaa
Copy link

Cabezaa commented Feb 22, 2018

Same problem that @BerndWessels here... Using MeteorJS (using uglify )

I try with graphql 0.12.3 and 0.12.0

@pleunv
Copy link

pleunv commented Feb 22, 2018 via email

benjamn added a commit to apollographql/graphql-js that referenced this pull request Jan 26, 2021
An improvement beyond graphql#1174, for non-production environments that enable
minification, such as NODE_ENV='staging'.

Since graphql-js goes to the trouble of providing a Symbol.toStringTag
property for most of the classes it exports, and that string is immune to
minification (unlike constructor.name), we should use it for error
checking in instanceOf(value, constructor) whenever the constructor
provides a tag, falling back to constructor.name and
value.constructor.name for constructors that do not define
Symbol.toStringTag (as before).

Motivating issue/investigation:
apollographql/apollo-client#7446 (comment)
@alistairholt
Copy link

I'm using 15.5.0 in a React Native project (Expo) and this is causing the built binaries to crash with the Ensure that there is only one instance of "graphql"… error. I'm assuming this is because of NODE_ENV environment variable check, which isn't set. I know there is only one one instance of graphql in node_modules. Frustrating.

benjamn added a commit to apollographql/graphql-js that referenced this pull request Mar 25, 2021
An improvement beyond graphql#1174, for non-production environments that enable
minification, such as NODE_ENV='staging'.

Since graphql-js goes to the trouble of providing a Symbol.toStringTag
property for most of the classes it exports, and that string is immune to
minification (unlike constructor.name), we should use it for error
checking in instanceOf(value, constructor) whenever the constructor
provides a tag, falling back to constructor.name and
value.constructor.name for constructors that do not define
Symbol.toStringTag (as before).

Motivating issue/investigation:
apollographql/apollo-client#7446 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.