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

Rename inspect methods #354

Closed
yousefamar opened this issue Mar 28, 2017 · 1 comment · Fixed by #355
Closed

Rename inspect methods #354

yousefamar opened this issue Mar 28, 2017 · 1 comment · Fixed by #355

Comments

@yousefamar
Copy link
Contributor

We were getting a weird crash at me-box/databox#4, and found the source after some sleuthing. To recreate:

new require('dockerode')().createNetwork({
	Name: 'n1',
	Driver: 'bridge'
})
.then((res)=>{
	console.log(res.toString());
})
.catch((err)=>{
	console.log('[ERROR]', err);
});

It turns out that .inspect() is reserved by Node, akin to .toString(), and is called when you try to log an object that implements it, and for debugging. This has caused issues in the past.

Now clearly things like Networks and Containers aren't meant to be console.log'd, and renaming the inspect methods would break backwards compatibility, but thought I should let you know anyway, and if you decide to rename them is up to you.

@yousefamar
Copy link
Contributor Author

yousefamar commented Mar 28, 2017

Alternatively, according to the docs, I believe you can also suppress this bug by adding a symbol-based inspect method to the classes, as that takes precedence, similar to this:

const util = require('util');
Network.prototype[util.inspect.custom] = function() { return this; };

yousefamar added a commit to yousefamar/dockerode that referenced this issue Mar 29, 2017
@yousefamar yousefamar mentioned this issue Mar 29, 2017
apocas added a commit that referenced this issue Apr 2, 2017
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 a pull request may close this issue.

1 participant