Skip to content

Remove class syntax for better babelification (2)#345

Merged
dcousens merged 1 commit intofeross:masterfrom
chjj:remove-class-syntax
Feb 5, 2024
Merged

Remove class syntax for better babelification (2)#345
dcousens merged 1 commit intofeross:masterfrom
chjj:remove-class-syntax

Conversation

@chjj
Copy link
Contributor

@chjj chjj commented Jan 31, 2024

Fixes the issues with #344.

@dcousens
Copy link
Collaborator

dcousens commented Feb 4, 2024

I'm not against this as the code is non-controversial, but if the intent is for working around Chrome 7, which best I can guess is from around 2010? That seems silly?

@chjj
Copy link
Contributor Author

chjj commented Feb 5, 2024

I can give a more realistic reason then: someone who is compiling a bundle and has a babel config set to anything that triggers pre-es2015 shims will receive a class shim from babel.

The babel class shims do not work when inheriting from any "special" builtin-object (Error, Array, TypedArray are examples). This is because they make the mistake I did originally and transform the constructor to follow the Base.call(this) pattern instead of the Object.setPrototypeOf(new Error(msg), NodeError.prototype) pattern this PR does.

In short, NodeError will not be an actual Error object if this occurs. That is, it won't have a stack trace, etc.

@chjj
Copy link
Contributor Author

chjj commented Feb 5, 2024

In any case, the code as it exists on master is incorrect. code should be an own/enumerable property for example. While this may seem minor, I imagine user's are used to seeing it in the console output:

$ node
Welcome to Node.js v21.6.1.
Type ".help" for more information.
> Buffer.alloc(1).write({})
Uncaught TypeError: argument must be a string
    at Buffer.write (node:buffer:1086:17) {
  code: 'ERR_INVALID_ARG_TYPE'
}

I don't think util.inspect displays the code property on errors unless it is enumerable.

Copy link
Collaborator

@dcousens dcousens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dcousens
Copy link
Collaborator

dcousens commented Feb 5, 2024

Thanks for the added context @chjj

@dcousens dcousens merged commit 7291685 into feross:master Feb 5, 2024
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.

2 participants