-
Notifications
You must be signed in to change notification settings - Fork 2.7k
new error extraction mechanism #10887
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
Conversation
🦋 Changeset detectedLatest commit: 4f3b270 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
/release:pr |
Please add a changeset via |
size-limit report 📦
|
/release:pr |
A new release has been made for this PR. You can install it with |
The released https://cdn.jsdelivr.net/npm/@apollo/[email protected]/invariantErrorCodes.js It boils down to: // This file is meant to help with looking up the source of errors like
// "Invariant Violation: 35" and is automatically generated by the file
// @apollo/client/config/processInvariants.ts for each @apollo/client
// release. The numbers may change from release to release, so please
// consult the @apollo/client/invariantErrorCodes.js file specific to
// your @apollo/client version. This file is not meant to be imported.
globalThis["ApolloErrorCodes_0.0.0-pr-10887-20230517113257"] = {
"@apollo/client version": "0.0.0-pr-10887-20230517113257",
1: {
file: "@apollo/client/cache/inmemory/entityStore.js",
condition: "typeof dataId === \"string\"",
message: "store.merge expects a string ID"
},
2: {
file: "@apollo/client/cache/inmemory/key-extractor.js",
condition: "extracted !== void 0",
message: "Missing field '%s' while extracting keyFields from %s"
},
// ...
} We can now create a HTML "error reader" file that imports this file from CDN and then accesses the error messages via |
/release:pr |
A new release has been made for this PR. You can install it with |
I changed the format of the I also added a global hook That way, error handling becomes opt-in instead of being opt-out. |
b305e44
to
36840da
Compare
Example link showing an error message from the CDN: The HTML for it is also not very complicated, we should be able to add that anywhere in our docs: https://github.com/phryneas/apollo-error-message-viewer/blob/main/index.html |
/release:pr |
A new release has been made for this PR. You can install it with |
All error warnings/messages are now extracted and can be loaded by import { loadErrorMessages, loadDevMessages } from "@apollo/client/dev";
loadErrorMessages();
// and/or
loadDevMessages(); or, more granularly, by a subset of import { devDebug, devError, devLog, devWarn, errorCodes } from "@apollo/client/invariantErrorCodes";
import { loadErrorMessageHandler } from "@apollo/client/dev";
loadErrorMessageHandler(devDebug, devError, devLog, devWarn, errorCodes);
|
Update: it does treeshake, next.js just doesn't display it nicely on build 🤦 Here are three sizes I got from building a Next.js app (minified, with and without gzip; I have no idea what else is in those bundles, so they are just a comparison to each other, total numbers mean nothing):
So in the end, in a "real life app", this change comes down to saving users 16kb (4.5kb gzipped), providing an opt-in solution, not an opt-out one. Next.js also nicely shakes
out on build, so we could document this approach for users. |
src/dev/loadErrorMessageHandler.ts
Outdated
} | ||
|
||
for (const codes of errorCodes) { | ||
Object.assign(global[ApolloErrorMessageHandler], codes) |
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 only problem I could maybe see would be if someone would use multiple versions of Apollo Client on the same website, and multiple of them would ship their error messages. In that case, those could override each other.
We could solve that by changing ApolloErrorMessageHandler from Symbol.for(...) to just Symbol() to make that unique per version, or by including version in the Symbol.for string - if you think that is necessary.
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.
I like this idea of making sure its different per version. Sounds like a small lift to make it work and would help us avoid (potentially) opened issues about this in the future.
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 - I did that now :)
class LinkError extends Error { | ||
public link?: ApolloLink; | ||
constructor(message?: string, link?: ApolloLink) { | ||
super(message); | ||
this.link = link; | ||
} | ||
} | ||
|
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.
This class was only created to be logged with invariant.warn
- it didn't have any more use beyond that.
* pretty-stringified objects. | ||
* Excess `optionalParams` will be swallowed. | ||
*/ | ||
function newInvariantError(message?: string | number, ...optionalParams: unknown[]) { |
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.
I had initially sub-classed the original InvariantError
here, but that is much more bundle size (as classes are downlevelled), so I opted for a wrapper function instead.
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.
[nit] Could we just call this invariantError
? Not sure we need the new
qualifier here. I find it to read a bit nicer:
if (somethingFailed) {
invariantError('You did it wrong');
}
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.
It made the change from throw new InvariantError
to throw newInvariantError
pretty easy - and usually, stuff we throw is a class constructor and needs to be new
ed, so I fear we would at some point instinctivly start calling the invariantError
function with the new
keyword.
So I'd opt for keeping it like this, but please tell me if you really can't live with it, then I'll change it :)
This part should be ready for review - I will have to adjust the HTML template to the latest changes and then will make PRs against the docs & shortlinks in parallel. |
/release:pr |
A new release has been made for this PR. You can install it with |
I was just shown this tool and it might actually come in handy when reviewing this PR: https://npmdiff.dev/%40apollo%2Fclient/3.8.0-alpha.15/0.0.0-pr-10887-20230522141246/package/apollo-client.cjs/ |
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.
This is incredible work! Really appreciate you taking the time to get this in time for v3.8! Apologies for my annoying style comments 🤣.
src/dev/loadErrorMessageHandler.ts
Outdated
} | ||
|
||
for (const codes of errorCodes) { | ||
Object.assign(global[ApolloErrorMessageHandler], codes) |
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.
I like this idea of making sure its different per version. Sounds like a small lift to make it work and would help us avoid (potentially) opened issues about this in the future.
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.
I'm super excited about this. Really appreciate the work you've done here!
This is just the current WIP status to see what we would generate on publish.
Checklist: