Import Node 10.9.0. Redo the fix for util.inspect.custom #357
Conversation
|
Is this still needed? readable-stream/test/browser.js Lines 16 to 20 in 2149b0d |
lib/internal/streams/buffer_list.js
Outdated
| var _require2 = require('util'), | ||
| inspect = _require2.inspect; | ||
|
|
||
| var custom = inspect && custom || Symbol('useless'); |
There was a problem hiding this comment.
Instead of using Symbol('useless') fall back to 'inspect'. That way old Node versions should work properly.
| const Buffer = require('buffer').Buffer | ||
| const util = require('util') | ||
| const { inspect } = require('util') | ||
| const custom = inspect && inspect.custom || 'inspect' |
There was a problem hiding this comment.
Won't this browserify all of util? I wrote https://github.com/mafintosh/inspect-custom-symbol to get around that (util is pretty big!)
There was a problem hiding this comment.
@mafintosh that module uses the symbol instead of 'inspect' as a key. See https://github.com/mafintosh/inspect-custom-symbol/blob/master/index.js#L4. @BridgeAR should we use the symbol or the property here?
There was a problem hiding this comment.
Also see #356 to remove the use of util.
There was a problem hiding this comment.
It should be 'inspect'. Otherwise old Node.js versions won't work and a symbol has no other effect on browserify in this case than the string.
I don't use browserify often enough to know exactly what it would pull in but it should only be the parts that are used and in this case it is only the inspection property. So that should be fine.
There was a problem hiding this comment.
I think we should land this as is and deal with removing util in #356. What do you think? May I get a couple of LGTM?
|
Added a comment |
Fixes #355