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

request: expose Node's error system #47521

Closed
cjihrig opened this issue Apr 12, 2023 · 13 comments
Closed

request: expose Node's error system #47521

cjihrig opened this issue Apr 12, 2023 · 13 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Apr 12, 2023

What is the problem this feature will solve?

It would allow userland to leverage core's error system. It would also make discussions around vendoring ecosystem modules into core go more smoothly (if package authors were willing to use it).

I am willing to do the work if people think this is a good idea.

What is the feature you are proposing to solve the problem?

Exposing the constructors for https://nodejs.org/api/errors.html#nodejs-error-codes to userland.

What alternatives have you considered?

Status quo

@cjihrig cjihrig added the feature request Issues that request new features to be added to Node.js. label Apr 12, 2023
@targos
Copy link
Member

targos commented Apr 12, 2023

Are there any downsides to doing it?

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 12, 2023

Nothing immediately stands out to me since the errors are pretty stable at this point. If there are individual errors that don't make sense to expose, we could discuss that as well.

@anonrig
Copy link
Member

anonrig commented Apr 12, 2023

Are there any downsides to doing it?

One downside is performance, especially node errors are very slow to create. nodejs/performance#40 written by @ronag

@ronag
Copy link
Member

ronag commented Apr 12, 2023

I think the Node error performance was significantly improved though. Don't remember which PR.

@anonrig
Copy link
Member

anonrig commented Apr 12, 2023

I think the Node error performance was significantly improved though. Don't remember which PR.

@ronag It's still open: #46648

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 12, 2023

@addaleax makes a good point on twitter regarding one issue with our current errors.

I would ... fix it first? The point of introducing it was to prevent users from parsing error message, but it's actually really not designed in a way that actually removes the need for that

like, now we have things like

E('ERR_NETWORK_IMPORT_DISALLOWED',
"import of '%s' by %s is not supported: %s", Error);

where if you want to get the value of any of the %s ... you need to *parse the error message*

Source: https://twitter.com/addaleax/status/1646155989261987840

@isaacs
Copy link
Contributor

isaacs commented Apr 12, 2023

I think @addaleax's comment is extremely valid, and it would be wonderful if this effort provided a motivation to polish up the error internals.

That said, whatever comes of this, I'd use the ever loving heck out of it, and I'd be motivated to polyfill in userland support for node versions that don't have it yet. I maintain a lot of fairly low-level utility libraries, and always try to follow Node's patterns wrt error codes and such, but it's always been a bit of a duck-typing mess. Exposing process.emitWarning was really nice, I see this as a natural evolution from that.

@mcollina
Copy link
Member

I created https://github.com/fastify/fastify-error to basically solve this problem. I can't find any issue, but I recall having some conversations with other folks and the general sentiment was that exposing it would not have been a good idea (e.g. small core).

I'm definitely +1, as it would one less module for me to maintain long term :D. We should also take the moment to polishing the internals and create some useful utilities.

On that note, we should include also utilities to generate warnings, as we had to write https://github.com/fastify/process-warning to match core behavior.

@ronag
Copy link
Member

ronag commented Apr 13, 2023

I'm +1.

@isaacs
Copy link
Contributor

isaacs commented Apr 13, 2023

On that note, we should include also utilities to generate warnings, as we had to write https://github.com/fastify/process-warning to match core behavior.

Yeah, process.emitWarning can be a bit clunky to use properly. But I think that probably ought to be a separate issue? Or is it connected to the error types in some way?

@mcollina
Copy link
Member

On that note, we should include also utilities to generate warnings, as we had to write https://github.com/fastify/process-warning to match core behavior.

Yeah, process.emitWarning can be a bit clunky to use properly. But I think that probably ought to be a separate issue? Or is it connected to the error types in some way?

The underlining mechanisms to provide a good DX are quite similar.

@cjihrig cjihrig closed this as completed Apr 14, 2023
@ronag
Copy link
Member

ronag commented Apr 14, 2023

Why was this closed?

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 14, 2023

I'm not going to work on this - there were some additional reservations about exposing this voiced by @joyeecheung on twitter - and I'm cleaning up my open issues list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

6 participants