FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

util: fix isBuffer by brendanashworth · Pull Request #1988 · nodejs/node · GitHub

/ node Public

util: fix isBuffer - #1988

Closed
brendanashworth wants to merge 3 commits into
nodejs:masterfrom
brendanashworth:fix/util-buffer
Closed

util: fix isBuffer#1988
brendanashworth wants to merge 3 commits into
nodejs:masterfrom
brendanashworth:fix/util-buffer

Conversation

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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 commented Jun 16, 2015

Copy link
Copy Markdown
Member

You can also put this line before the requires

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

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Contributor

@brendanashworth #1892 should fix that eventually.

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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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/

Copy link
Copy Markdown
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

Copy link
Copy Markdown
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
PR-URL: #1988
Reviewed-By: Roman Reiss <me@silverwind.io>
brendanashworth added a commit that referenced this pull request Jun 16, 2015
PR-URL: #1988
Fixes: #1987
Reviewed-By: Roman Reiss <me@silverwind.io>
brendanashworth added a commit that referenced this pull request Jun 16, 2015
PR-URL: #1988
Reviewed-By: Roman Reiss <me@silverwind.io>

Copy link
Copy Markdown
Contributor Author

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL