-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
@apollo/client v3 broken in Rollup & Snowpack #6352
Comments
related: #6035 |
@FredKSchott can you post an example of the failures you're seeing? I haven't tried Snowpack, but AC3 definitely works with Rollup. Thanks! |
Good catch @FredKSchott. It's disappointing that the only way to defer an ESM Please do give us your thoughts on how we might avoid importing |
Yup, 100% agreed and wish this was better supported in ESM. But even Webpack is limited by how much it can statically analyze. Bundlers can see that @apollo/client is importing & requiring React, but they can't tell at build-time wether that runtime function that does it will or won't ever be run, so a bundler will normally just bundle "react" into your app just to be safe. Based on the above, I'm 99% sure that Webpack is bundling I'd recommend:
|
Thanks @FredKSchott, we'll look into this further. Just a small note though - AC3's current lazy react |
@hwillson FWIW, the example you are citing is I did a workaround though to make it work with Rollup/React: I created a rollup plugin which replaces the lazy require with a static import.
|
Yup, that's correct, the lazy require isn't called in that example. |
I really like the second option from @FredKSchott! |
Also about ES Modules related,
I have noticed this trying to make |
There is a 4th way too, which can achieve conditional bundling without asking users to use a different namespace to import from: Have 2 modules: // src/react/react.ts
let React: typeof import('react');
export function requireReactLazily(): typeof import('react') {
if(!React) {
throw new Error('React not loaded, consider putting `import @apollo/client/react/load`');
}
return React;
}
export function setReact(_React) {
React = _React;
} And another, // src/react/load.ts
import React from 'react';
import { setReact } from './react';
setReact(React); Now, have the users when using // app.tsx
import { ApolloClient, ApolloProvider, useQuery } from '@apollo/client';
import '@apollo/client/react/load'; // This is required only once.
... Rest of the App ... Instead of |
Thanks all - sorry, I misinterpreted the original problem regarding the example app I shared (I was focused on the use case of not needing React). We had discussed a lot of the ideas in this thread back when we decided to integrate our React hooks into the |
@hwillson I think that's a good call 👍 |
to help prevent modern bundlers from requiring React when used with `@apollo/client/core` (in other words, without any of Apollo Client's React components). While this approach works well for applications that aren't using React, it introduces problems for bundlers and applications that are using React (as outlined in #6035 and #6352). There are several different ways we can address this, and we might do something more substantial in the future, but for now this commit manipulates Apollo Client's core CJS bundle at build time, to make the React require optional. Fixes #6035. Fixes #6352.
PR #5577 introduced a new way of lazily loading React to help prevent modern bundlers from requiring React when used with `@apollo/client/core` (in other words, without any of Apollo Client's React components). While this approach works well for applications that aren't using React, it introduces problems for bundlers and applications that are using React (as outlined in #6035 and #6352). There are several different ways we can address this, and we might do something more substantial in the future, but for now this commit manipulates Apollo Client's core CJS bundle at build time, to make the React require optional. Fixes #6035. Fixes #6352.
Excellent! Thanks for the quick fix. What's the best way to test this in Snowpack/Rollup? |
@FredKSchott we'll publish a new |
@FredKSchott we just published |
That worked! Thanks again |
Tracked it back to this item:
https://github.com/apollographql/apollo-client/blob/master/src/react/react.ts#L8
The require function doesn't exist inside an ES Module (any file with
import
orexport
syntax), which is causing the package to fail in some bundlers.Happy to help brainstorm ideas on how to solve if you'd like, but would strongly recommend against shipping 3.0 with broken support in any major bundlers.
The text was updated successfully, but these errors were encountered: