-
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
Revamp CJS and ESM bundling with Rollup (extracted from #4261) #4401
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks Ben! I can only spot one thing to change based on our conversations elsewhere - removal of browser
from all the package.json files
@hwillson I still need to write some changelog notes about this PR, but I think it's ready for review (whenever you get to it next week). |
Nice, I am definitely in favor of a cjs bundle for main. Bundles for all will kill interop problems. |
Yeah, thanks for continuing to push for that. I’m sold now! |
Great to see the consistency becoming reality in all packages now, only graphql-tag left. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this originally @rosskevin, and revamping it @benjamn!
To create this commit, I (@benjamn) first squashed all of @rosskevin's commits from PR #4261 into one big commit, then reverted any changes that did not seem essential to the new bundling strategy discussed in that PR. Once that was done, I rebased against origin/release-2.5.0 and fixed the conflicts. With a few more minor tweaks, all tests are passing. I will push a few additional changes after this commit, but I wanted to preserve @rosskevin's intentions as much as possible here, since his name is still attached to this commit.
Type checking should respect the local tsconfig.json file within each package.
f283ca7
to
8a79339
Compare
1070b6e
to
8cb112c
Compare
@@ -136,7 +139,7 @@ export default class ApolloClient<TCacheShape> implements DataProxy { | |||
} | |||
|
|||
if (!link || !cache) { | |||
throw new Error(` | |||
throw new InvariantError(` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the key benefits of ts-invariant
over invariant
is that the InvariantError
class is exposed, so it can be thrown in a way that TypeScript understands (for type-narrowing purposes). The rollup-plugin-invariant
transform also knows how to strip this message string in production (because I wrote both packages).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love ts-invariant
!
invariant( | ||
options.query, | ||
'query option is required. You must specify your GraphQL document ' + | ||
'in the query option.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The preconditions in this function are a good example of over-checking for hypothetical mistakes (if we keep going down this road, where does it end?), but at least we don't have to pay for these error strings in production anymore!
Mixing default and named imports causes Rollup to use both, which leads to slightly more generated code, on the whole.
@hwillson Ready for another look I think! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @benjamn - thanks!
@@ -136,7 +139,7 @@ export default class ApolloClient<TCacheShape> implements DataProxy { | |||
} | |||
|
|||
if (!link || !cache) { | |||
throw new Error(` | |||
throw new InvariantError(` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love ts-invariant
!
To create this PR, I (@benjamn) first squashed all of @rosskevin's commits from PR #4261 into one big commit, then reverted any changes that did not seem essential to the new bundling strategy discussed in that PR. Once that was done, I rebased against
origin/release-2.5.0
and fixed the conflicts. With a few more minor tweaks, all tests are now passing (locally, at least).I will push a few additional changes after this commit, but I wanted to preserve @rosskevin's intentions as much as possible here, since his name is still attached to this commit.