-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Change BaseException prototype #18003
Conversation
While developing a website using SvelteKit I started getting errors when trying to use pdfjs. However the error came from vite, because it could not properly handle the error thrown by pdfjs. After debugging for a bit I found that the root of the issue was that vite assumes a "stack" property on errors, which is fair considering that pretty much every implementation has that, but the error thrown did not have this property. After changing vite's error handling a bit I found the real underlying issue - just a simple [MissingPDFException: Missing PDF "...".] because I messed up the path. I reverted my changes to vite's error handling and started looking for the cause of it not working as intended when I found the line `BaseException.prototype = new Error();` which looked weird to me and after a bit of googling I found [this StackOverflow](https://stackoverflow.com/questions/783818/how-do-i-create-a-custom-error-in-javascript) question where people recommended using `CustomError.prototype = Object.create(Error.prototype)`. I quickly tested it and now I directly got the exception printed in console, not something unrelated. Being satisfied with finding the cause I then fixed the path to the PDF being wrong and now it is working as intended. Just wish I would not have wasted this much time on an issue this simple.
One option is to use proper subclassing: class BaseException extends Error {
constructor(message, name) {
if (new.target === BaseException) {
unreachable("Cannot initialize BaseException.");
}
super(message);
this.name = name;
}
} I assume that this was originally written not as a class because the code is transpiled with Babel and Babel did not support extending built-ins, but this has changed long ago (babel/babel#7020, released in Babel 7.0.0). EDIT: The reason was SystemJS (#11185), but it's mot used anymore (#12563) :) |
Unfortunately I don't believe that that actually works though. I tried something similar, a while back, and it broke structured cloning of Errors (during postMessage) and caused various test failures. |
To be honest while I do have decent experience with JS, I know pretty little about classes and prototypes. There might be a better solution that what I did, what I did might break in some other edge case, I don't know. I just found a solution that fixed the issue I encountered and I did not find any comment about it breaking elsewhere. |
I have looked into this a bit more because the reported problem and solution intrigued me, but sadly I don't think we can accept this change because it doesn't seem to work correctly (see below), and to be honest I also don't understand why this would work in the first place. The suggestion from this PR seems to originate from https://issues.chromium.org/issues/41005493, but that bug was already fixed years ago so it shouldn't impact modern browsers. Furthermore, if I check the In conclusion, unless it can be shown that the If this breaks SvelteKit, Vite or any other third-party framework for that matter, then to me that sounds like that framework is somehow acting incorrectly, and that's usually not something we can fix here (it also wouldn't be the first time we've seen frameworks doing "creative" things with e.g. built-in object prototypes). Nevertheless, thank you for filing this issue because it did uncover that we miss unit test coverage for this! |
The issue from mozilla#18003 hasn't been shown to be caused by PDF.js, but it did surface that we don't have (direct) unit test coverage for the `BaseException` class. This made it more difficult to prove that the `stack` property was already available on exception instances, but more importantly it caused the CI to be green even though the suggested change would have caused the `stack` property to disappear. To avoid future regressions, for e.g. similar changes or a rewrite from a closure to a proper class, this commit introduces a dedicated unit test for `BaseException` that asserts that our exception instances indeed expose all expected properties.
While developing a website using SvelteKit I started getting errors when trying to use pdfjs. However the error came from vite, because it could not properly handle the error thrown by pdfjs.
After debugging for a bit I found that the root of the issue was that vite assumes a "stack" property on errors, which is fair considering that pretty much every implementation has that, but the error thrown did not have this property. After changing vite's error handling a bit I found the real underlying issue - just a simple [MissingPDFException: Missing PDF "...".] because I messed up the path.
I reverted my changes to vite's error handling and started looking for the cause of it not working as intended when I found the line
BaseException.prototype = new Error();
which looked weird to me and after a bit of googling I found this StackOverflow question where people recommended usingCustomError.prototype = Object.create(Error.prototype)
. I quickly tested it and now I directly got the exception printed in console, not something unrelated.Being satisfied with finding the cause I then fixed the path to the PDF being wrong and now it is working as intended. Just wish I would not have wasted this much time on an issue this simple.