The v3.4.0-beta.15 doesn't work buildless

[abdonrd]

Intended outcome:

Just working, as previous version.

Actual outcome:

I get an error when update from v3.4.0-beta.14 to v3.4.0-beta.15:

This file is: https://github.com/graphql/graphql-js/blob/v15.2.0/src/jsutils/instanceOf.js#L14
But that file hasn't changed since January 2020.

All works if I go back to v3.4.0-beta.14.

I'm using the @web/dev-server.

How to reproduce the issue:

Just importing and setup Apollo Client.

Versions

It happens only in v3.4.0-beta.15 and v3.4.0-beta.16.
All works if I go back to v3.4.0-beta.14.

Reference: #7399

[abdonrd]

This downgrade makes the project work again.

[benjamn]

@abdonrd Thanks for testing the betas and reporting this issue!

This is a consequence of apollographql/invariant-packages#94, and it unfortunately reveals that the graphql package is not designed to be used without a build step, since it contains a naked reference to the global process variable. This is a common assumption in the React ecosystem, for better or worse.

Does @web/dev-server have any build/minification options for replacing expressions like process.env.NODE_ENV with constant strings? If not, you may need to provide your own global process.env polyfill to keep using the graphql package without the global.process stub that ts-invariant was previously providing.

The reason graphql/graphql-js#2894 is relevant is that @IvanGoncharov mentioned they might stop using process.env.NODE_ENV in graphql@16, which would be a welcome improvement, since it would allow graphql to run in a browser without a build step.

[bennypowers]

@web/dev-server doesn't make any assumptions. Users can add plugins or configure the test runner to window.process ??= { env: { NODE_ENV: 'development' } } or something, but why should lighterweight approaches have to give way to build-heavy tooling-required usages? I hope that in the near future using apollo and graphql will only be more and more inclusive and accessible.

It would be a welcome improvement for graphql library to drop the dependency on node globals and adopt a definition of "isomorphic" as "runs in node and browsers" instead of as "runs in node and node"

[benjamn]

@bennypowers No disagreement here—you're preaching to the choir!

We are actively looking for a way to stop relying on process.env.NODE_ENV within Apollo Client, though every approach so far seems to require asking developers to add additional Apollo-specific build/minifier/polyfill configurations, whereas process.env.NODE_ENV has the dubious benefit of already being required by the React ecosystem.

While I am confident we will find a satisfactory solution within the @apollo/client package, this will continue to be a problem as long as any of our dependencies are making naked process references (graphql and react being the two main offenders as of this writing).

[benjamn]

A possible workaround here would be to define window.process yourself, before any code tries to access it. If you want to find out who's accessing it, you can use a property getter function that logs when accessed:

if (typeof process === "undefined") {
  const processStub = { env: {} };
  Object.defineProperty(globalThis, "process", {
    get() {
      console.log("accessing globalThis.process!");
      console.trace();
      return processStub;
    }
  });
}   

[benjamn]

@abdonrd I believe this is fixed now (as of @apollo/client@3.4.0-rc.10)! #8266 (comment)

Thanks so much for helping test the beta releases, and for your patience while we fixed this.

[abdonrd]

Thanks @benjamn! Happy to help!

[hwillson]

Just echoing @benjamn's sentiments here @abdonrd - we can't thank you (and anyone else in the community doing the same) enough for testing beta releases! 🙇‍♂️

[bennypowers]

👋 hey I'm noticing some breaking changes to TS types:

import type { MutationUpdaterFn } from '@apollo/client/core';

became

import type { MutationUpdaterFunction } from '@apollo/client/core';

and RefetchQueriesDescription was removes from @apollo/client/core/watchQueryOptions

that last one could arguably be considered internal and non-breaking, but the first one should be aliases before releasing, I think

edit: also looks like the type signature for FetchMoreQueryOptions changed

[benjamn]

@bennypowers I hear you about MutationUpdaterFn (see b677262), but the other two don't worry me as much. You'll have to pay attention (only) if you were using those types in your own code, but the improvements are worth the attention.

Do these seem to be the only visible type changes? Were you able to compare the output of tsc in a systematic way?

[bennypowers]

Thanks!

Yeah, Those were the only ones I found in apollo-elements, which tries its best to expose the full breadth of the core apis

@abdonrd shared this screenshot with me, which I'm reposting with permission

I reproduced this locally, then after confirming those 8 errors, I published a fix with some workarounds, mostly by reexporting type aliases. apollo-elements/apollo-elements@30a31ea see especially packages/core/types.ts in that diff.

I haven't gone over /link and friends as carefully (and certainly not /react), but updating some of my demo apps so far has been smooth. I wouldn't typically expect app-buildery kind of users to go mucking around too much with those types.

Thanks :D