Node: errors in punycode.js: assign codes or leave them alone?

[joyeecheung]

From https://github.com/nodejs/node/issues/11273 which is almost complete except this one.

All the errors thrown from there are RangeErrors for invalid input.

[bnoordhuis]

_Pro_: easy change to make, all exceptions are thrown from the error() function.

_Con_: it's a third-party dependency that we vendor in, and deprecated to boot.

Personally, I'd be okay with floating a small patch. It's unlikely to be a maintenance hassle.

[BridgeAR]

which is almost complete except this one

We still throw regular errors in https://github.com/nodejs/node/blob/bb98f271815f6f1357c3c7178a3367d439c44029/lib/internal/modules/cjs/loader.js#L685-L689 (even though we attach an error code but one that is not aligned with all other error codes). I wonder if we should ever port that to internal/errors as well?

[joyeecheung]

@BridgeAR I guess the answer from my side is “if it does not involve any circular dependency zalgo (which I don’t think there is) then why not”.

Also this makes good first contributions so that’s a plus.

[targos]

Isn't the issue with MODULE_NOT_FOUND that this code is used since forever (I could find it in v0.10) and it would be a major breaking change for the ecosystem to change it?

[BridgeAR]

@targos yes, I also checked 0.8 and it's even used in there. I also checked Gzemnid and userland seems to check for the code frequently. I guess we could open a couple PRs to change the check to check for two codes and as soon as most modules adopted to that strategy to change the error code but otherwise it's hard to do that. We could also stick to the current error code but still migrate it to our internal errors (just with the old error code that doesn't match the new ones).

[jasnell]

I would leave punycode.js untouched as it is deprecated and only used by node.js when node.js is built without ICU.

[jasnell]

There's been no further activity on this. Closing. If necessary we can revisit this