Avoid relying on constructor.name for instanceOf error check. #2894
Conversation
|
That code coverage failure is reasonable! |
|
@benjamn Thanks for PR. Currently, we are focused on TS migration so let's keep this PR open until we figure out a future-proof solution. |
|
@IvanGoncharov Dropping Would you consider making an update to the 15.x.x line, given that no (pre)release of 16.x is available yet? This is a real problem for Apollo Client developers, and I don't know of any good workarounds. Here's one that I suggested recently, if you want to see what I mean. I would also caution against following the lead of the React repo, since they are still using a conditional |
|
@IvanGoncharov Do you think you could take another look at this PR, in light of my previous comment? Thanks! |
|
@benjamn Will take a look into this. |
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)
After rebasing PR graphql#2894 against origin/main, I noticed that the src/polyfills/symbols.js module no longer exists, and Symbol.toStringTag is used directly elsewhere in the codebase.
benjamn
force-pushed
the
avoid-constructor.name-in-instanceOf
branch
from
823d311 to
717f24e
Compare
|
@IvanGoncharov Just rebased against |
|
@benjamn Did another quick review and agree that it should be merged. |
|
👋 @IvanGoncharov - do you think this will make it into the next alpha? |
As explained in #8266, our use of process.env.NODE_ENV requires those expressions to be either replaced with a string literal by a minifier (common convention in the React ecosystem) or polyfilled globally. We stopped polyfilling process.env globally in the ts-invariant package in apollographql/invariant-packages#94, but @apollo/client is still relying on process.env internally, as is the graphql package. If we want to rid ourselves fully of the drawbacks of process.env.NODE_ENV, we probably ought to stop using it ourselves. Though most developers in the React ecosystem will be using a bundler or minifier that replaces process.env.NODE_ENV at build time, you may not have the luxury of custom minification when loading @apollo/client from a CDN, which leaves only the option of a global process polyfill, which is problematic because that's how some applications detect if the current code is running Node.js (more details/links in #8266). Instead, I believe we can (and must?) stop using process.env.NODE_ENV, and one of many better alternatives appears to be the __DEV__ variable used by React Native, which is much easier to polyfill, since it's a single variable rather than a nested object. Switching to __DEV__ will initially cause a large bundle size regression (+3.5Kb *after* minification and gzip), but that's not technically a breaking change (and thus acceptable for AC3.4), and it should be easy to reconfigure minifiers to replace __DEV__ with true or false (or even just undefined), with sufficient guidance from the release notes. That still leaves the process.env.NODE_ENV check in instanceOf.mjs in the graphql package. Discussion in graphql/graphql-js#2894 suggests the plan is to stop using NODE_ENV altogether, which would be ideal, but that won't happen until graphql@16 at the earliest. To work around the problem in the meantime, I devised a strategy where we polyfill process.env.NODE_ENV only briefly, while we import code that depends on graphql/jsutils/instanceOf.mjs, and then synchronously remove the global polyfill, so it does not permanently pollute the global namespace. This strategy assumes @apollo/client is the first to import the graphql package. If you have other code that imports instanceOf.mjs earlier, and you don't have a process.env.NODE_ENV strategy already, it's your responsibility to make that import work, however you see fit. Apollo Client is only responsible for making sure its own imports of the graphql package have a chance of succeeding, a responsibility I believe my strategy fulfills cleverly if not elegantly. Of course, this charade does not happen if process.env.NODE_ENV is already defined or has been minified away, but only if accessing it would throw, since that's what we're trying to avoid. Although we could do some more work to reduce the bundle size impact of blindly switching to __DEV__, I believe this PR already solves the last remaining hurdles documented in #8266, potentially allowing @apollo/client/core@beta to be loaded from an ESM-aware CDN like esm.run or jspm.io. The default __DEV__ value will be true in those environments, but could be initialized differently by a script/module that runs earlier in the HTML of the page.
As explained in #8266, our use of process.env.NODE_ENV requires those expressions to be either replaced with a string literal by a minifier (common convention in the React ecosystem) or polyfilled globally. We stopped polyfilling process.env globally in the ts-invariant package in apollographql/invariant-packages#94, but @apollo/client is still relying on process.env internally, as is the graphql package. If we want to rid ourselves fully of the drawbacks of process.env.NODE_ENV, we probably ought to stop using it ourselves. Though most developers in the React ecosystem will be using a bundler or minifier that replaces process.env.NODE_ENV at build time, you may not have the luxury of custom minification when loading @apollo/client from a CDN, which leaves only the option of a global process polyfill, which is problematic because that's how some applications detect if the current code is running Node.js (more details/links in #8266). Instead, I believe we can (and must?) stop using process.env.NODE_ENV, and one of many better alternatives appears to be the __DEV__ variable used by React Native, which is much easier to polyfill, since it's a single variable rather than a nested object. Switching to __DEV__ will initially cause a large bundle size regression (+3.5Kb *after* minification and gzip), but that's not technically a breaking change (and thus acceptable for AC3.4), and it should be easy to reconfigure minifiers to replace __DEV__ with true or false (or even just undefined), with sufficient guidance from the release notes. That still leaves the process.env.NODE_ENV check in instanceOf.mjs in the graphql package. Discussion in graphql/graphql-js#2894 suggests the plan is to stop using NODE_ENV altogether, which would be ideal, but that won't happen until graphql@16 at the earliest. To work around the problem in the meantime, I devised a strategy where we polyfill process.env.NODE_ENV only briefly, while we import code that depends on graphql/jsutils/instanceOf.mjs, and then synchronously remove the global polyfill, so it does not permanently pollute the global namespace. This strategy assumes @apollo/client is the first to import the graphql package. If you have other code that imports instanceOf.mjs earlier, and you don't have a process.env.NODE_ENV strategy already, it's your responsibility to make that import work, however you see fit. Apollo Client is only responsible for making sure its own imports of the graphql package have a chance of succeeding, a responsibility I believe my strategy fulfills cleverly if not elegantly. Of course, this charade does not happen if process.env.NODE_ENV is already defined or has been minified away, but only if accessing it would throw, since that's what we're trying to avoid. Although we could do some more work to reduce the bundle size impact of blindly switching to __DEV__, I believe this PR already solves the last remaining hurdles documented in #8266, potentially allowing @apollo/client/core@beta to be loaded from an ESM-aware CDN like esm.run or jspm.io. The default __DEV__ value will be true in those environments, but could be initialized differently by a script/module that runs earlier in the HTML of the page.
|
Awesome @IvanGoncharov - thanks very much! |
|
@benjamn @hwillson Release as https://github.com/graphql/graphql-js/releases/tag/v16.0.0-alpha.4 |
|
@IvanGoncharov I just tried this code and I get an error: I'm running node v16.3.0 as I had to upgrade to take this update. edit: oh whoops, this is already taken care of in 40db639. sorry to bother you, eagerly awaiting a new version of graphql.js :D |
|
I backported this in #3186 so we can get this fix on v15.x. Not much of the tooling has caught up to v16 yet so this should allow the change to work for a lot more people. |
PR #1174 disabled
instanceOferror checking whenprocess.env.NODE_ENV === 'production', which eliminated some false positives due to collisions of minified class names in production. This PR aims to eliminate the same kind of false positive errors in non-production environments that enable minification, such asNODE_ENV='staging'. Please see apollographql/apollo-client#7446 (comment) for motivating use cases and background explanation.Fortunately, the
graphqlpackage goes to the trouble of providing aSymbol.toStringTagproperty for most of the classes it exports (one of many examples), and that string is immune to minification (unlikeconstructor.name). This PR proposes using that tag string for error checking ininstanceOf(value, constructor)whenever theconstructorprovides aSymbol.toStringTagproperty, falling back toconstructor.nameandvalue.constructor.namefor constructors that do not defineSymbol.toStringTag(as before).If this is controversial in any way, I would be happy to discuss alternative solutions, but I believe this change will resolve many of the remaining issues linked at the bottom of the #1174 issue thread.
Remaining to-dos:
instanceOftests to restore full coverage.