Skip to content
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

fix: assign message to error object in handle_error using Object.assign #11675

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented May 17, 2024

This uses Object.defineProperty to write to the error message instead.

Copy link

changeset-bot bot commented May 17, 2024

🦋 Changeset detected

Latest commit: 4a89e28

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

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

@Rich-Harris
Copy link
Member

I just ran into this locally while looking into something else and was able to solve it with Object.defineProperty — any reason not to use that instead?

@trueadm
Copy link
Contributor Author

trueadm commented May 19, 2024

Does that work for workers too?

@Rich-Harris
Copy link
Member

The behaviour I see is quite strange. With a new Error('original message'):

  • in Firefox and Safari overwriting the message works just fine
  • in Chrome I can set the value, and it updates (console.log(e.message) reflects the new value), but the error that's printed to the console contains the original message. Presumably something to do with how the error is cloned (though if I manually structuredClone the error, it works)

The place where I originally saw this was with a DOMException (in the main thread), where the message is a getter on the prototype rather than a property of the error object:

try {
  document.createComment('').appendChild(document.createElement('div'));
} catch (e) {
  console.log(Object.getOwnPropertyDescriptor(e, 'message')); // undefined

  e.message = 'overwritten'; // has no effect
  console.log(e.message); // original message

  Object.defineProperty(e, 'message', { value: 'overwritten' }); 
  console.log(e.message); // 'overwritten'

  throw e; // throws 'overwritten' in Chrome and Safari, original message in Firefox
}

So there's absolutely no consistency between browsers, they're all just making it up as they go along. I'm not sure how to create a DOMException in a worker where there's no DOM; things like SyntaxError which I imagined would be similar are actually more like conventional errors.

In other words I can't answer your question without knowing what error you were dealing with in the first place, because normal errors seem to work just fine (except that the overwritten message is ignored in some situations).

@trueadm trueadm changed the title fix: avoid writing to not writable error object in handle_error fix: assign message to error object in handle_error using Object.assign May 21, 2024
@trueadm
Copy link
Contributor Author

trueadm commented May 21, 2024

I switched to using Object.assign

.changeset/shiny-pillows-relax.md Outdated Show resolved Hide resolved
packages/svelte/src/internal/client/runtime.js Outdated Show resolved Hide resolved
packages/svelte/src/internal/client/runtime.js Outdated Show resolved Hide resolved
@Rich-Harris
Copy link
Member

did you test this with the error you encountered with a non-writable message?

trueadm and others added 2 commits May 21, 2024 14:50
Co-authored-by: Rich Harris <rich.harris@vercel.com>
Co-authored-by: Rich Harris <rich.harris@vercel.com>
@trueadm
Copy link
Contributor Author

trueadm commented May 21, 2024

@Rich-Harris I'll try and find out from the user who reported it.

trueadm and others added 2 commits May 21, 2024 14:51
Co-authored-by: Rich Harris <rich.harris@vercel.com>
Co-authored-by: Rich Harris <rich.harris@vercel.com>
@petermakeswebsites
Copy link

petermakeswebsites commented May 26, 2024

It doesn't seem to be fixed. I created a laughably minimal repro. I guess the error is still thrown by the worker, even though one might expect it to be thrown by the main thread...

Here is a something similar to what I had in my original code of the error in case its useful to have more substance.

EDIT: I notice for some reason the stack trace might not be showing in the REPL, even though the error does. Here is my local dev stack trace:

Uncaught (in promise) TypeError: Cannot set property message of  which has only a getter
    at handle_error (runtime.js:288:8)
    at execute_effect (runtime.js:503:3)
    at flush_queued_effects (runtime.js:568:4)
    at flush_queued_root_effects (runtime.js:548:5)
    at flush_sync (runtime.js:745:4)
    at flush_sync (runtime.js:752:4)
    at mount (render.js:106:9)
    at new Svelte4Component (legacy-client.js:75:49)
    at new <anonymous> (legacy-client.js:47:4)
    at initialize (client.js:434:9)

runtime.js 287-290

const indent = /Firefox/.test(navigator.userAgent) ? '  ' : '\t';
error.message += `\n${component_stack.map((name) => `\n${indent}in ${name}`).join('')}\n`;
      ^^^^^^^
const stack = error.stack;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants