chore: avoid Object.assign

[Uzlopak]

Resolves #ISSUE_NUMBER

---

Before the change?

After the change?

Pull request checklist

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• Yes
• No

---

[github-actions]

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

[G-Rath]

this change does more than just remove the Object.assign - could you retitle your PR to be either a fix or feat, and provide more explanation on why you think this change is needed (even better if you could add a test related to it)

[Uzlopak]

Why is it doing more than removing Object.assign? It is the same behaviour than before. event and status are not non-wrtieable or non-configurable propertie on a generic Error. And Object.assign makes them also enumerable. Obviously Object.assign is slightls slower than just assigning the properties.

[G-Rath]

• you're adding a new property to the types
• you're making the properties not enumerable (which is technically a breaking change, but it's probably fine)
• performance improvements are not without actual numbers to back them up

I'm not saying this change shouldn't be landed, but there is a reason we have a PR template

[Uzlopak]

Ok, I have now to check if assigning with Object.assign is different then directly assigning the properties. Thank you for this precious hint. No sarcasm or so. I am quiet impressed regarding this aspect I did not see.

[Uzlopak]

@G-Rath

I checked it, and directly assigning makes them also enumerable. So there is no breaking change.

const bla = { a: 1}

Object.assign(bla, { b: 2 });

bla.c = 3;

Object.defineProperty(bla, 'd', {
  value: 4,
  enumerable: false,
});

console.log(bla.propertyIsEnumerable('a')); // true
console.log(bla.propertyIsEnumerable('b')); // true
console.log(bla.propertyIsEnumerable('c')); // true
console.log(bla.propertyIsEnumerable('d')); // false

And yes, mutating is always faster. Object. assign is actually mutating the Object, thus if you would use a Proxy you could see that the setters are triggered on the Object.

The type change actually just demonstrates, that the properties were missing on the type. The Object.assign actually hid the the fact that those propertes were not documented.

[wolfy1339]

This would be a bug fix for the types.

Is it really faster to not use Object.assign ?
At what point does it really matter?

[Uzlopak]

You are right. We have other bottlenecks. Actually verification could/should be improved.

When I started working on #1167 i opened my local project and saw these changes, and i thought, that I should propose it. Object.assign swallowed the issue of lack of typings. So this was the reason why i wrote that it avoids Object.assign. The slightly faster remark in my first answer was just a clarifying that it has also some kind of perf effect. But it was not my main target.

I can add back the Object.assign, if you like.

[wolfy1339]

I believe that the fact that this fixes a type issue and fixes the fact that the properties were hidden from typescript and thus the type issue not detected, make this PR just fine

Concerns addressed

[github-actions]

🎉 This PR is included in version 14.1.3 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀