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

Modifying a stack descriptor may break descriptor pre-requisites. #6

Closed
Qix- opened this issue Oct 31, 2016 · 7 comments
Closed

Modifying a stack descriptor may break descriptor pre-requisites. #6

Qix- opened this issue Oct 31, 2016 · 7 comments
Assignees
Labels

Comments

@Qix-
Copy link
Owner

Qix- commented Oct 31, 2016

As noted in sindresorhus/parse-json#6, if the stack element descriptor already has a value then adding a get() to it will cause it to fail:

        Object.defineProperty(this, 'stack', stackDescriptor);
               ^

TypeError: Invalid property descriptor. Cannot both specify accessors and a value or writable attribute, #<Object>
@FrancescoCioria
Copy link

Hi @Qix- I'm having this exact error 100% of times when using node@7 (precisely 7.4.0)

Do you know any workaround for this?

@Qix-
Copy link
Owner Author

Qix- commented Jan 25, 2017

Yes and no. The fix seemed easy enough, but alas it wasn't. I have to investigate a bit further but node 7 broke a bunch of stuff. :|

@roberttod
Copy link

https:/Qix-/node-error-ex/pull/7 seems to fix this issue for me, using node 7.6.

@Qix- could you publish master to npm? Or is there another issue I am missing?

@Qix-
Copy link
Owner Author

Qix- commented Mar 3, 2017

@roberttod There was a problem with the fix, or something else was happening. Let me take a look.

@Qix-
Copy link
Owner Author

Qix- commented Mar 3, 2017

Alright, a build is underway. When it clears, I'll publish and write up a post-mortem. It might even result in a bug report over at Node... Not sure how strict the ES specification is (I'm not super familiar with the ES6 specification).

tl;dr Node 7 started caching messages in stacks rather than building them dynamically. Also, it appears they use ES6 classes internally for Error which broke inheriting. That was an easy fix thanks to Domenic Demicola.

@Qix-
Copy link
Owner Author

Qix- commented Mar 3, 2017

Published as 1.3.1. Yes, a bugfix. Thanks for everyone that nagged me on this; I was procrastinating on this for far too long.

Postmortem to follow (not required reading).


Post-Mortem

Ugh.

So I'm not entirely sure what happened, or what caused this from Node v6 to Node v7. I just know something happened in their definition of Error.

I can only guess that they did the equivalent of replacing function Error() {} with class Error {} in Node v7, and in the process changed the logic that builds up the stack trace when retrieving the value of .stack.

@boehm-s's PR (#7) fixed only a part of the problem, but the overall issue was a bit deeper. Their PR was green because the .travis.yml in this repository had an outdated list of node versions to test against, which is my fault. This has been remedied with adding stable along with 5, 6, and 7 (overkill but why not, right?).

Alright so my theory as to what happened: historically, accessing .stack would dynamically build up the first line of the stack trace (which changed dynamically with error-ex's .append() properties), meaning whenever we accessed .stack, the first line would be correct even if you changed the message/error-ex property values between references of the .stack property.

Still following? Yeah, it gets a bit hairy. Needless to say this was a confusing bug.

In Node v7, I speculate that they switched out the implementation for a newer ES6 implementation that no longer dynamically builds up .stack, but instead caches the return value for subsequent references to the .stack properties.

This would explain why first there were descriptor issues, why inheritance broke (since util.inherits() doesn't work when you're inheriting a function from an ES6 class - see below), and why the third part of the break involved the messages not being built up correctly.

For the inquisitive, here is the subtle documentation regarding the change in semantics for inheritance using util.inherits():

Note: usage of util.inherits() is discouraged. Please use the ES6 class and extends keywords to get language level inheritance support. Also note that the two styles are semantically incompatible.

That was stupid annoying to fix - not because it was difficult but because nothing made any sense.

That's what you get for writing a module that is essentially a hack ;) Hopefully everything should be back to normal without updating any package.json files.

Let me know if this doesn't fix it! I'll be closely monitoring this repository to make sure the bugfix didn't actually break anyone (as everything should be backwards compatible). Please open a ticket if you see something break as a result of the 1.3.1 release.

// cc @sindresorhus @indutny (maybe you have some insight about this) @FrancescoCioria @dominicbarnes @roberttod @typicode

@Qix- Qix- closed this as completed Mar 3, 2017
@roberttod
Copy link

roberttod commented Mar 4, 2017

@Qix- clearly then there was an issue I missed! Thanks for resolving this and for the incredibly detailed post mortem.

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

No branches or pull requests

3 participants