Skip to content

Conversation

@PepijnSenders
Copy link

@PepijnSenders PepijnSenders commented Feb 4, 2022

The current logic causes the following warning:

Cannot assign to read only property 'call' of object '#<Object>'

There's no need to do a .call() here we can just use argument spread.

if (originalConsoleLevel) {
Function.prototype.apply.call(originalConsoleLevel, global.console, args);
if (originalConsoleLevel && 'originalConsoleLevel' in global.console) {
global.console[originalConsoleLevel](...args);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Unfortunately, we can't accept it, because it both causes a build error and breaks the functionality of the code.

Remember that originalConsoleLevel is a function, which is why the build throws this error:

image

Further, originalConsoleLevel is not a property of global.console. I suspect you're thinking of level instead, which is the name of the method in question ("warn", "error", and so on), which IS a property of global.console. That said, changing originalConsoleLevel in your second line to level also doesn't fix it, because now global.console[level] is the wrapped function, not the original.

All of that said:

  1. I can understand how you got confused, because originalConsoleLevel sounds like it should be a string. I'll give it a better name.

  2. You're not wrong that it can be simplified: it can be changed to originalConsoleLevel.call(global.console, args);, which I will also do.

Cheers!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation, I didn't test or check the types yet (just made this in Github editor as an example). Thanks for taking it over and applying this change, this will save some errors in our sentry :)

lobsterkatie added a commit that referenced this pull request Feb 4, 2022
…e` (#4505)

In responding to #4504, I noticed two things which could be improved about the code in question:

1) The author of that PR was misled by the admittedly confusing name of the original method we're wrapping.

2) We've been calling that method in a more complex way than necessary.

This fixes both of those problems.
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

Successfully merging this pull request may close these issues.

2 participants