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

t.throws throws with "Cannot delete property" #320

Closed
dcousens opened this issue Sep 9, 2016 · 8 comments
Closed

t.throws throws with "Cannot delete property" #320

dcousens opened this issue Sep 9, 2016 · 8 comments

Comments

@dcousens
Copy link

dcousens commented Sep 9, 2016

Reference: https://github.com/substack/tape/blob/master/lib/test.js#L448

        delete err.message;
               ^

TypeError: Cannot delete property 'message' of Error
    at Test.throws (../node_modules/tape/lib/test.js:448:16)

This seems like really odd behaviour and doesn't work with custom errors?

@ljharb
Copy link
Collaborator

ljharb commented Sep 9, 2016

f66724f implies that the purpose is ensuring that the property is enumerable (even in ES3).

It should work fine with custom errors, unless their "message" property is nonconfigurable for some reason. What test code is creating this error?

@dcousens
Copy link
Author

dcousens commented Sep 9, 2016

The error is coming from https://github.com/dcousens/typeforce/blob/master/index.js#L13-L20, as I'm putting that repository under tape at the moment.

Even ensuring the property is enumerable: true doesn't solve my issue?
It is lazily evaluated due to tfErrorString performing JSON.stringify a few times.

@ljharb
Copy link
Collaborator

ljharb commented Sep 9, 2016

So, a few reactions:

  • you should probably use Object.defineProperty inside your getter to change it into a data property once it's cached instead of making it always be a (slow) getter
  • since the goal of that delete + reassign of "message" was enumerability, I think a) it should not do that if the property is already enumerable, and b) it should detect if the property is nonconfigurable, and avoid attempting to modify it if that's the case.

@dcousens
Copy link
Author

dcousens commented Sep 9, 2016

you should probably use Object.defineProperty inside your getter to change it into a data property once it's cached instead of making it always be a (slow) getter

That was my first thought, but node 6 doesn't allow that? (or am I doing something else wrong?)

TypeError: Cannot redefine property: message

since the goal of that delete + reassign of "message" was enumerability, I think a) it should not do that if the property is already enumerable, and b) it should detect if the property is nonconfigurable, and avoid attempting to modify it if that's the case.

Couldn't you just check that via:

assert(Object.keys(err).some(x => x === 'message'))

@ljharb
Copy link
Collaborator

ljharb commented Sep 9, 2016

Even easier, and ES3-compatible, with Object.prototype.isEnumerable.call. I'll make that change tomorrow, at least.

Because configurable defaults to false, you'll have to change the getter defineProperty to have configurable: true to be able to redefine it later.

@dcousens
Copy link
Author

dcousens commented Sep 9, 2016

Thanks mate, so for tomorrows change, I'll be able to get away with changing: enumerable: true for my purposes?

@ljharb
Copy link
Collaborator

ljharb commented Sep 9, 2016

Yep, that's the idea.

@ljharb ljharb closed this as completed in c1b483c Sep 10, 2016
@dcousens
Copy link
Author

Thanks @ljharb :)

ljharb added a commit that referenced this issue Sep 30, 2016
 - [Fix] `throws`: only reassign “message” when it is not already non-enumerable (#320)
 - [Fix] show path for error messages on windows (#316)
 - [Fix] `.only` should not run multiple tests with the same name (#299, #303)
 - [Deps] update `glob`, `inherits`
 - [Dev Deps] update `concat-stream`, `tap`, `tap-parser`, `falafel`
 - [Tests] [Dev Deps] Update to latest version of devDependencies tap (v7) and tap-parser (v2) (#318)
 - [Tests] ensure the max_listeners test has passing output
 - [Docs] improvements (#298, #317)
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

No branches or pull requests

2 participants