Skip to content

fix: Uncaught TypeError: Illegal invocation in chrome 51 - #290

Merged
ericyhwang merged 2 commits into
share:masterfrom
qinyang912:master
May 15, 2019
Merged

fix: Uncaught TypeError: Illegal invocation in chrome 51#290
ericyhwang merged 2 commits into
share:masterfrom
qinyang912:master

Conversation

@qinyang912

Copy link
Copy Markdown
Contributor

in chrome 51, will throw error

image

in chrome 51, will throw error
Uncaught TypeError: Illegal invocation
@coveralls

coveralls commented May 9, 2019

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.005%) to 95.914% when pulling 7c729d7 on qinyang912:master into ca4816f on share:master.

Comment threadlib/logger/logger.js Outdated
SUPPORTED_METHODS.forEach(function (method) {
if (typeof overrides[method] === 'function') {
logger[method] = overrides[method];
logger[method] = overrides[method].bind(overrides);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice, but shouldn't this be:

Suggested change
logger[method]=overrides[method].bind(overrides);
logger[method]=overrides[method].bind(overrides[method]);

And by that point, maybe we can tidy up the loop a bit:

SUPPORTED_METHODS.forEach(function(method){varoverride=overrides[method];if(typeofoverride==='function'){logger[method]=override.bind(override);}});

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

should be this:

SUPPORTED_METHODS.forEach(function(method){varoverride=overrides[method];if(typeofoverride==='function'){logger[method]=override.bind(overrides);}});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why bind to the object of overrides rather than the function itself? Or actually, maybe more accurate would be:

logger[method]=override.bind(logger);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

you can try these code in chrome 51:

varlogger={};varoverride=console.log;logger.log=override.bind(override);logger.log('test')

and

varlogger={}varoverride=console.loglogger.log=override.bind(logger)logger.log('test')

both throw error
image

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

but this is successful:

varlogger={};varoverride=console.log;logger.log=override.bind(console);logger.log('test');

image

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You're getting that error because of this issue with Chrome. The correct thing to do is:

varlogger={};varoverride=console.log.bind(console);logger.log=override;logger.log('test');

@alecgibson

Copy link
Copy Markdown
Collaborator

I'm closing this, because the correct usage should be:

ShareDB.logger.setMethods({log: console.log.bind(console),});

@alecgibson

Copy link
Copy Markdown
Collaborator

Sorry, I just realised you mean this happens with the default Logger?

I'm not sure that arbitrarily setting the this argument to the provided object is valid. Instead, we should change how we set the default:

functionLogger(){vardefaultMethods={};SUPPORTED_METHODS.forEach(function(method){// Deal with Chrome issue: https://bugs.chromium.org/p/chromium/issues/detail?id=179628defaultMethods[method]=console[method].bind(console);});this.setMethods(defaultMethods);}

@alecgibsonalecgibson reopened this May 10, 2019
@qinyang912

Copy link
Copy Markdown
ContributorAuthor

@alecgibson Yes, I mean the default logger.. 😄

@ericyhwang
ericyhwang merged commit b0d4277 into share:masterMay 15, 2019
@ericyhwang

Copy link
Copy Markdown
Contributor

Thanks! Published in sharedb@1.0.0-beta.23

Sign up for freeto 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.

4 participants

@qinyang912@coveralls@alecgibson@ericyhwang