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.

Sign up for GitHub

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

Jump to bottom

Readonly error.code has ecosystem impact. #15658

Closed
jdalton opened this issue Sep 28, 2017 · 10 comments
Closed

Readonly error.code has ecosystem impact. #15658

jdalton opened this issue Sep 28, 2017 · 10 comments
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core.

Comments

@jdalton
Copy link
Member

jdalton commented Sep 28, 2017 •
edited
Loading

I know internal/errors usage is picking up in Node core so I thought I'd give a heads up on something I've seen in the wild. It looks like folks are adding error.code properties which will error with the internal/errors since error.code is readonly.

Is being readonly intentional?
@jdalton jdalton mentioned this issue Sep 28, 2017
Closed
@targos
Copy link
Member

targos commented Sep 28, 2017

There was some discussion about that in the PR introducing internal/errors: #11220 (review)

/cc @jasnell

@targos targos added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Sep 28, 2017
@jasnell
Copy link
Member

jasnell commented Sep 28, 2017

Yes, the read only was intentional in order to explicitly prevent blind overriding. I believe the property is still configurable tho. It's been a while tho so I may be wrong

@jdalton
Copy link
Member Author

jdalton commented Sep 28, 2017 •
edited
Loading

Yes, the read only was intentional in order to explicitly prevent blind overriding.

This applies to CJS code as well, right?

Is there a user-land scenario in mind, around augmenting Node specific errors, that you're wanting to prevent?

I'm wondering if the user-land code should have to worry about it. To user-land code an error object is an error object. It could be a syntax error, or some other kind of error, and now code will have to branch for Node specific error objects or handle all error objects as tricky ones (which I'd assume is not the usual case).

@jasnell
Copy link
Member

jasnell commented Sep 28, 2017

I'm not against changing it. There is a side effect in that the code is used in the name property, so it's not entirely free

@jdalton
Copy link
Member Author

jdalton commented Sep 28, 2017 •
edited
Loading

There is a side effect in that the code is used in the name property, so it's not entirely free

The symbol prop is used in the name so .code and .name can be modified independently.

@evanlucas
Copy link
Contributor

I would be +1 on changing. I've seen usage of overwriting Error#code some.

@jasnell
Copy link
Member

jasnell commented Sep 28, 2017

What I mean is that changing code would make it out of sync with the name. We should decide whether those should be kept in sync automatically.

@jdalton
Copy link
Member Author

jdalton commented Sep 28, 2017

We should decide whether those should be kept in sync automatically

Since the .code and .name props aren't tied together (.name doesn't rely on .code) I think it'd be fine to allow augmenting to be yolo (getting too fancy with syncing seems like unnecessary overhead).

@jasnell
Copy link
Member

jasnell commented Sep 28, 2017

That's the answer I'd be hoping for ;)

@jdalton jdalton mentioned this issue Sep 28, 2017
Closed
@Trott Trott mentioned this issue Sep 30, 2017
Closed
3 tasks
@Trott
Copy link
Member

Trott commented Sep 30, 2017

Proposed quick fix in #15694

Trott added a commit to Trott/io.js that referenced this issue Sep 30, 2017
@Trott
errors: make properties mutable
7436ac7 
Userland code can break if it depends on a mutable `code` property for
errors. Allow users to change the `code` property but do not propagate
changes to the error `name`.

Additionally, make `message` and `name` consistent with `Error` object
(non-enumerable). Test that `console.log()` and `.toString()` calls on
internal `Error` objects with mutated properties have analogous results
with the standard ECMAScript `Error` objects.

Fixes: nodejs#15658
Trott added a commit to Trott/io.js that referenced this issue Oct 4, 2017
@Trott
errors: make properties mutable
7eddd5e 
Userland code can break if it depends on a mutable `code` property for
errors. Allow users to change the `code` property but do not propagate
changes to the error `name`.

Additionally, make `message` and `name` consistent with `Error` object
(non-enumerable). Test that `console.log()` and `.toString()` calls on
internal `Error` objects with mutated properties have analogous results
with the standard ECMAScript `Error` objects.

Fixes: nodejs#15658
Trott pushed a commit to Trott/io.js that referenced this issue Oct 4, 2017
@jdalton @Trott
error: make code and name properties settable
9efeea3 
For internal errors, make `code` and `name` settable while keeping them
non-own properties by default.

Fixes: nodejs#15658
Trott pushed a commit to Trott/io.js that referenced this issue Oct 4, 2017
@jdalton @Trott
errors: make code and name properties settable
42a2a9b 
For internal errors, make `code` and `name` settable while keeping them
non-own properties by default.

PR-URL: nodejs#15694
Fixes: nodejs#15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@Trott Trott closed this as completed in 6e172be Oct 4, 2017
@Trott Trott mentioned this issue Oct 8, 2017
Closed
3 tasks
addaleax pushed a commit to addaleax/ayo that referenced this issue Oct 12, 2017
@Trott @addaleax
errors: make properties mutable
cfd039a 
Userland code can break if it depends on a mutable `code` property for
errors. Allow users to change the `code` property but do not propagate
changes to the error `name`.

Additionally, make `message` and `name` consistent with `Error` object
(non-enumerable). Test that `console.log()` and `.toString()` calls on
internal `Error` objects with mutated properties have analogous results
with the standard ECMAScript `Error` objects.

PR-URL: nodejs/node#15694
Fixes: nodejs/node#15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
addaleax pushed a commit to addaleax/ayo that referenced this issue Oct 12, 2017
@jdalton @addaleax
errors: make code and name properties settable
b512d68 
For internal errors, make `code` and `name` settable while keeping them
non-own properties by default.

PR-URL: nodejs/node#15694
Fixes: nodejs/node#15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Trott added a commit to Trott/io.js that referenced this issue Oct 12, 2017
@Trott
errors: make properties mutable
eb7c6dc 
Userland code can break if it depends on a mutable `code` property for
errors. Allow users to change the `code` property but do not propagate
changes to the error `name`.

Additionally, make `message` and `name` consistent with `Error` object
(non-enumerable). Test that `console.log()` and `.toString()` calls on
internal `Error` objects with mutated properties have analogous results
with the standard ECMAScript `Error` objects.

PR-URL: nodejs#15694
Fixes: nodejs#15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Trott pushed a commit to Trott/io.js that referenced this issue Oct 12, 2017
@jdalton @Trott
errors: make code and name properties settable
d4436a0 
For internal errors, make `code` and `name` settable while keeping them
non-own properties by default.

PR-URL: nodejs#15694
Fixes: nodejs#15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
evanlucas pushed a commit that referenced this issue Oct 23, 2017
@Trott @evanlucas
errors: make properties mutable
0bd2fd5 
Userland code can break if it depends on a mutable `code` property for
errors. Allow users to change the `code` property but do not propagate
changes to the error `name`.

Additionally, make `message` and `name` consistent with `Error` object
(non-enumerable). Test that `console.log()` and `.toString()` calls on
internal `Error` objects with mutated properties have analogous results
with the standard ECMAScript `Error` objects.

Backport-PR-URL: #16078
PR-URL: #15694
Fixes: #15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
evanlucas pushed a commit that referenced this issue Oct 23, 2017
@jdalton @evanlucas
errors: make code and name properties settable
939a580 
For internal errors, make `code` and `name` settable while keeping them
non-own properties by default.

Backport-PR-URL: #16078
PR-URL: #15694
Fixes: #15658
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

errors: make code property mutable Trott/io.js
5 participants
@jdalton @jasnell @evanlucas @Trott @targos