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

change in v5: const {inspect} = require('util'); can lead to web bundle bloat #26

Closed
dylang opened this issue Sep 24, 2019 · 7 comments · Fixed by #27
Closed

change in v5: const {inspect} = require('util'); can lead to web bundle bloat #26

dylang opened this issue Sep 24, 2019 · 7 comments · Fixed by #27

Comments

@dylang
Copy link

dylang commented Sep 24, 2019

This was added in v5 for deserializeError:

const {inspect} = require('util');

This adds about 8KB to web bundles, which can be significant, especially in projects not using deserializeError.

Possible solutions:

  • Not worry about 8KB and tell developers to use v4 if bundle size is of concern.
  • Separate serializeError into a separate file that can be imported without the deserializeError code.
  • Separate deserializeError into a new module.
  • Publish code with ES6-style import, add sideEffects: false to the package.json to enable tree shaking in the module.
  • Other ideas?
@sindresorhus
Copy link
Owner

sindresorhus commented Sep 24, 2019

I didn't realize people used this in the browser.

We could maybe add {"browser": {"util": false}} to package.json and check for its existence before using it? However, then the output will not be consistent between Node.js and the browser. Not sure that matters. Or we could find a simpler way to show the type. It's not important. It's just a bonus to make it easier to debug passed values that are not really deserializable as errors.

We should probably also guard

Error.captureStackTrace(this, NonError);
then, as it doesn't exist in Firefox.

@rohan-deshpande
Copy link

Thanks for this issue. I think this is a really handy lib and making it universal is a good idea! I support this approach:

Or we could find a simpler way to show the type. It's not important. It's just a bonus to make it easier to debug passed values that are not really deserializable as errors.

Basically the only reason for using inspect here is to be able to find out what happened right? Just for debugging purposes? I think if there is some light weight way to implement this that'd be ideal, maybe just use JSON.stringify if it's an object?

@sindresorhus
Copy link
Owner

Yeah, we could probably just use JSON.stringify.

@vladimiry
Copy link
Contributor

vladimiry commented Mar 10, 2020

I didn't realize people used this in the browser.

Using deserializeError in browser is a quite common case. Like when you serialize the error on backed and serve it to in-browser client or main process => renderer process interaction in the case of @electron IPC use.

Such change seems to be working well.

@sindresorhus
Copy link
Owner

@vladimiry A PR would be welcome :)

@vladimiry
Copy link
Contributor

Going to adjust tests and PR then.

@vladimiry
Copy link
Contributor

PR placed: #27.

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

Successfully merging a pull request may close this issue.

4 participants