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

Use serialize-error for error serialization #594

Open
safareli opened this issue Jun 20, 2022 · 3 comments
Open

Use serialize-error for error serialization #594

safareli opened this issue Jun 20, 2022 · 3 comments

Comments

@safareli
Copy link

If you are using custom error classes with custom properties or even more or less standard cause then you would loose some data here:

comlink/src/comlink.ts

Lines 249 to 256 in 4ba8162

serialized = {
isError: true,
value: {
message: value.message,
name: value.name,
stack: value.stack,
},
};

Instead I'm proposing to use serialize-error that works better then what we have currently.

I can create PR if this is approved.

Meanwhile you can do this as a workaround

@benjamind
Copy link
Collaborator

Given the workaround is sufficient here, and adding the serialization would for most folks be unnecessary additional payload do you think this is actually worth adding to the library, or should we just document the workaround for those folk that need it?

@safareli
Copy link
Author

The workaround depends on internal api and needs unsafe type casts. it would be nice to just use serialize-error but if adding dependency is a problem then making API of swapping out transferHandlers in a nicer way would be one option.

Maybe some sort of plugin api could be implemented here. and there would be a plugin that uses serialize-error for errors

@benjamind
Copy link
Collaborator

I guess I missed the usage of an internal api, which part uses that?

If you want to put up a PR to use serialize-error so we can compare payload impact that'd be the best path forward here. Besides that I think we would need more indication of this being a consistent problem for users to determine if its worth addng the additional payload for this.

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

No branches or pull requests

2 participants