Instable instanceof check

[Netail]

Hi, We've encountered an issue where the following would always result in true (throwing the error); https://github.com/apollographql/apollo-client-nextjs/blob/main/packages/client-react-streaming/src/DataTransportAbstraction/WrapApolloProvider.tsx#L71-L75

We have client wrapper package which configures a default apollo client set up for all of our frontends, this package gets transpiled into commonjs. Because this package imports the .cjs files of client-react-streaming (and thus use the apolloclient from commonjs) when transpiled and our frontends use the provider from ESModules (thus expecting the apolloclient from ESModules), it won't match the instanceof. Throwing the following; Error: When using ApolloClient in streaming SSR, you must use the NextSSRApolloClient export provided by "@apollo/experimental-nextjs-app-support/ssr".

Ofcourse we can transpile the package to ESModules instead, but a different way of checking if it's a streaming apolloclient would be appreciated :)

[phryneas]

Hi @Netail, I empathize with you, but I'm not sure of it's a good idea if we actually did that.

Of course, we could add a different mechanism of detecting that here, but I don't think we'd actually do you a service with that:
You have a setup that will ship two copies of the same library. This might not only cause all kinds of unforeseen bugs (like this one, but probably a lot more as soon as the graphql package gets involved), it also means that you send twice the bundle size over the wire, which you should really aim to avoid.

This error is an indicator that something is very wrong with your bundling setup, and that you really need to get that under control. (A lot more could break in a lot more subtle ways.) So... is there any way you could fix your bundling setup instead? I know it's not the answer you want to hear, but I don't feel very comfortable with adding another workaround around that here tbh.

[Netail]

Well it's not necessarily bundled with the package, it's an optional peerDependency, so it uses the same version as the one used in the project

[phryneas]

But you're shipping one CJS copy of the whole package and one ESM copy of the whole package, or you wouldn't see this problem, right?

[Netail]

No, it doesn't ship with the package at all. Our custom wrapper package uses the .cjs files from the streaming package, while the app uses the .js files of the streaming package. I'll try to set up a demo, might be a bit easier to explain

[phryneas]

Yeah, that might help.

That said, even running both the CJS and ESM version of the same package is dangerous and should be avoided, as packages can have internal state, and you're creating two versions of that state that way - and they might (not) interact with each other in unexpected ways.

You can read up more on that under Dual Package hazard.

If this is a problem you can fix on your side, you should really do it - this is not only about this package, but also about every other package you're using.

[Netail]

Yeah, that might help.

That said, even running both the CJS and ESM version of the same package is dangerous and should be avoided, as packages can have internal state, and you're creating two versions of that state that way - and they might (not) interact with each other in unexpected ways.

You can read up more on that under Dual Package hazard.

If this is a problem you can fix on your side, you should really do it - this is not only about this package, but also about every other package you're using.

Ah yes, that's exact the problem I am currently facing. I'll try to transpile to ESM

[Netail]

Mhm seems like it's still throwing Error: When using ApolloClient in streaming SSR, you must use the NextSSRApolloClient export provided by "@apollo/experimental-nextjs-app-support/ssr". with only using ESM

[phryneas]

Could you provide me some kind of reproduction for that? I'd love to take a look.

[Netail]

Could you provide me some kind of reproduction for that? I'd love to take a look.

Turns out it was because of the dynamic import, this could be solved by making it async, but this is not allowed in the makeClient for the provider

[phryneas]

I'm not exactly sure what you mean, can you show me?

[Netail]

I'm not exactly sure what you mean, can you show me?

So as the nextjs support is optional, we want to make the import optional, as we also use the same client builder in non-nextjs projects. We first used experimentalNextSupport = require('@apollo/experimental-nextjs-app-support/ssr') with importing, which uses the commonjs client, so I switch to ESM importing. However, with this change, experimentalNextSupport is used before it is defined. We could make it async and change the dynamic import to experimentalNextSupport = import('@apollo/experimental-nextjs-app-support/ssr'), but that would mean the makeClient in the apollo provider needs to support promises

export function createClient(options: CreateClientOptions) {
    let experimentalNextSupport;
    if (options.nextjs) {
        try {
            import('@apollo/experimental-nextjs-app-support/ssr')
                .then(e => experimentalNextSupport = e);
        } catch (err) {
            console.error('[AH-Graphql-Client] Failed to import @apollo/experimental-nextjs-app-support');
        }
    }

    const cache = options.nextjs && !!experimentalNextSupport
        ? new experimentalNextSupport.NextSSRInMemoryCache({
            possibleTypes: options.possibleTypes,
            typePolicies: typePolicies,
            ...options.cacheOptions,
        })
        : new InMemoryCache({
            possibleTypes: options.possibleTypes,
            typePolicies: typePolicies,
            ...options.cacheOptions,
        });

    // More client setup incl SSRClient & Multilink dependent on nextjs option

    return client;
}

[phryneas]

Ah :/ Unfortunately, supporting an async version of makeClient is not a simple task - we execute that within a React Component. So we'd essentially need to add suspense just for makeClient - and adding any kind of suspense is pretty much only possible if we have a "per-request global store", which is the thing we actually want to create with makeClient.

One thought: Could you maybe instead of that nextjs option pass that '@apollo/experimental-nextjs-app-support/ssr' import as an argument into createClient, from the consuming code (because that can do a synchronous import)?

[Netail]

I guess that could work, but feels kind of hacky?

[phryneas]

Alternatively, you can probably use a top-level await in the module itself, but then you need something else to decide if it should be loaded, e.g. an environment variable.

It's unfortunate, but we really cannot make that function async without blowing up complexity immensely.

[Netail]

Ahh oke, no worries just a thought. We currently resolved this with your solution by passing the import to the package, but not the most beautiful solution. Thanks for thinking with me :)

[phryneas]

Sure :)

I'm going to close this, but please feel free to reopen (or open a new issue) if something comes up.

[github-actions[bot]]

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.