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

util: fix isBuffer #1988

Closed

Conversation

brendanashworth
Copy link
Contributor

Importing Buffer caused a circular dependency issue. We need to use the
globally exported variable to get around this.

Fixes: #1987

This is just quick patch to fix the bug, I hope we can later find a better solution.

@brendanashworth
Copy link
Contributor Author

sigh, this spurs a linter error:

lib/util.js
  665:24  error  Use const Buffer = require('buffer').Buffer; at the beginning of this file  require-buffer

✖ 1 problem (1 error, 0 warnings)

cc @silverwind

@targos
Copy link
Member

targos commented Jun 16, 2015

You can also put this line before the requires

@mscdex mscdex added the util Issues and PRs related to the built-in util module. label Jun 16, 2015
@brendanashworth brendanashworth force-pushed the fix/util-buffer branch 2 times, most recently from 2456ba4 to d1b5247 Compare June 16, 2015 04:44
@brendanashworth
Copy link
Contributor Author

Still hacky, but better! Thanks @targos, I switched the order of exports.

I tried to remove the isBuffer call altogether (keep it DRY with buffer.isBuffer), but that causes another circular dependency. I think the best way forward is to move util.deprecate to an internal module.

@brendanashworth
Copy link
Contributor Author

Alright, force pushed a fix that moves deprecate() to an internal module, allowing us to circumvent the whole thing. Right now, the internal module .deprecate is still exposed through util.deprecate.

@silverwind
Copy link
Contributor

@brendanashworth #1892 should fix that eventually.

As for the linter error, you could just put /* eslint-disable require-buffer */ in worst case.

@silverwind
Copy link
Contributor

Also I wonder why no test has caught this circular dependency. util.isBuffer seems completely untested, maybe add a small test?

@brendanashworth
Copy link
Contributor Author

@silverwind hmm, looking at #1892, it would reintroduce the dep issue because the internal module references back to the util module. Do you think I should just add the linter error hack for now, have #1892 fix the cyclic issue, and call it a day? And I added a small true / false test, I too was surprised it wasn't caught by the test suite.

Added a small commit, summary:

> util.isBuffer === Buffer.isBuffer
true

@silverwind
Copy link
Contributor

I'd be fine with a hack if you denote it as such (// HACK:). #1892 still needs a bit of work.

@brendanashworth
Copy link
Contributor Author

I'd prefer to go through with these commits, without introducing a hack / linter comment, to fix the bug. #1892 shouldn't be hard to port the changes through, as the location of the code is all that changes.

@silverwind
Copy link
Contributor

True, shouldn't be too hard to have separate methods later.

LGTM, started off a CI just to be sure:

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/17/

@silverwind
Copy link
Contributor

CI failures are unrelated. @brendanashworth can you land it?

This and 671e64a are pretty important fixes and we should probably do a quick patch release for them.

cc: @rvagg @Fishrock123 @chrisdickinson

@brendanashworth
Copy link
Contributor Author

@silverwind yes, landing now - 3806d87 seems to be important too, they were talking about a CVE.

brendanashworth added a commit that referenced this pull request Jun 16, 2015
brendanashworth added a commit that referenced this pull request Jun 16, 2015
PR-URL: #1988
Fixes: #1987
Reviewed-By: Roman Reiss <[email protected]>
brendanashworth added a commit that referenced this pull request Jun 16, 2015
PR-URL: #1988
Reviewed-By: Roman Reiss <[email protected]>
@brendanashworth
Copy link
Contributor Author

Thank you, I've merged these commits in 671e64a...626432d, in time for #1996.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

util.isBuffer(...) fails on 2.3.0
4 participants