Boomify throws when using multiple versions of Boom

AdriVanHoudt commented 3 years ago

Support plan

is this issue currently blocking your project? (yes/no): no
is this issue affecting a production system? (yes/no): it was

Context

node version: 14.x
module version with issue: 9.1.3
last module version without issue: based on https://github.com/hapijs/boom/commit/13bf5b2e1b02cbf0677ce3bae37eab28edf14991 I'd say 7.4.5
environment (e.g. node, browser, native): node
used with (e.g. hapi application, another framework, standalone, ...):
any other relevant information:

What are you trying to achieve or the steps to reproduce?

"use strict";
const Boom = require('@hapi/boom');
const Boom2 = require('somedep/node_modules/@hapi/boom'); // This illustrates a boom error being made in a dependency somewhere, whereas Boom is our app's version 

const err = Boom2.boomify(new Error('jos'), { statusCode: 400 });
const err2 = Boom.boomify(err, { statusCode: 500 });
console.log(err2);

// throws `TypeError: Cannot redefine property: reformat`

What was the result you got?

TypeError: Cannot redefine property: reformat

What result did you expect?

it would just boomify again and change the statusCode

I think just adding writable: true here https://github.com/hapijs/boom/blob/master/lib/index.js#L424 would solve the issue. The defineProperty was added here https://github.com/hapijs/boom/commit/13bf5b2e1b02cbf0677ce3bae37eab28edf14991 to solve enumarable but auto added the non-writeable part.

Let me know if you want me to do a PR.

kanongil commented 3 years ago

It should just need configurable: true (writable is for regular assignments).

I was not aware that the Cannot redefine property error only triggers when the value is different and not on the defineProperty() call itself.

kanongil commented 3 years ago

Checking the ES spec, it is indeed allowed to call defineProperty() if all values of the descriptor match the current one, regardless of the value of configurable. Additionally, it is allowed to change a writable: true to writable: false, even if configurable: false.

kanongil commented 3 years ago

Regarding how to fix this, making it configurable only fixes the case where an old version creates a boom object from a new version. The new version would still crash if fed an old version, which would still not be configurable. But it will work if it is just a de-duplication thing.

AdriVanHoudt commented 3 years ago

yeah in my specific case we had 2 copies of 9.1.3, just not deduped by npm (which is fixed by rebuilding the lockfile from scratch).

I see your point of old/new versions but it still makes it safer to use (and then more so in the future).

kanongil commented 3 years ago

I agree. This is a corner case and making it configurable fixes it for the future. Besides, we don't support old versions.

AdriVanHoudt commented 3 years ago

Submitted a PR here: https://github.com/hapijs/boom/pull/289

hapijs / boom