avoid importing util #83

Merged
merged 3 commits into from Mar 29, 2014

Conversation

Projects
None yet
5 participants
@calvinmetcalf
Member

calvinmetcalf commented Mar 5, 2014

as mentioned in #82 this avoids importing util in favor of directly using the shim modules, waiting on sam-github/node-debuglog#1 to avoid having that bring it in as well.

@TooTallNate

This comment has been minimized.

Show comment Hide comment
@TooTallNate

TooTallNate Mar 5, 2014

Contributor

This probably needs to be implemented in the transform scripts, no? /cc @rvagg

Contributor

TooTallNate commented Mar 5, 2014

This probably needs to be implemented in the transform scripts, no? /cc @rvagg

@maxogden

This comment has been minimized.

Show comment Hide comment
@maxogden

maxogden Mar 5, 2014

Contributor

does someone wanna write a contributing section in the readme that explains the whole transform thing and how to run/test/send PRs?

Contributor

maxogden commented Mar 5, 2014

does someone wanna write a contributing section in the readme that explains the whole transform thing and how to run/test/send PRs?

@calvinmetcalf

This comment has been minimized.

Show comment Hide comment
@calvinmetcalf

calvinmetcalf Mar 5, 2014

Member

tests I actually did run and it passes (now), transform thing totally in the dark about through

Member

calvinmetcalf commented Mar 5, 2014

tests I actually did run and it passes (now), transform thing totally in the dark about through

@calvinmetcalf

This comment has been minimized.

Show comment Hide comment
@calvinmetcalf

calvinmetcalf Mar 5, 2014

Member

debuglog was updated so the git dep has been replaced by a real npm version

Member

calvinmetcalf commented Mar 5, 2014

debuglog was updated so the git dep has been replaced by a real npm version

@calvinmetcalf

This comment has been minimized.

Show comment Hide comment
@calvinmetcalf

calvinmetcalf Mar 6, 2014

Member

ok on reading the repo more carefully it looks like this needs to go into the build folder somewhere

Member

calvinmetcalf commented Mar 6, 2014

ok on reading the repo more carefully it looks like this needs to go into the build folder somewhere

@rvagg

This comment has been minimized.

Show comment Hide comment
@rvagg

rvagg Mar 22, 2014

Owner

I'm 👍 on inherits() but 👎 on debuglog. An additional dependency just for debugging is a bit much. If you want it and util.debuglog doesn't exist then just patch util before loading reqdable-stream; that ought to be satisfactory for debugging situations.

Owner

rvagg commented Mar 22, 2014

I'm 👍 on inherits() but 👎 on debuglog. An additional dependency just for debugging is a bit much. If you want it and util.debuglog doesn't exist then just patch util before loading reqdable-stream; that ought to be satisfactory for debugging situations.

@calvinmetcalf

This comment has been minimized.

Show comment Hide comment
@calvinmetcalf

calvinmetcalf Mar 22, 2014

Member

so leave the debuglog stuff alone and add browser:{util:false} to the
readme ?

On Fri, Mar 21, 2014 at 10:50 PM, Rod Vagg notifications@github.com wrote:

I'm [image: 👍] on inherits() but [image: 👎] on debuglog. An
additional dependency just for debugging is a bit much. If you want it and
util.debuglog doesn't exist then just patch util before loading
reqdable-stream; that ought to be satisfactory for debugging situations.

Reply to this email directly or view it on GitHubhttps://github.com/isaacs/readable-stream/pull/83#issuecomment-38341105
.

-Calvin W. Metcalf

Member

calvinmetcalf commented Mar 22, 2014

so leave the debuglog stuff alone and add browser:{util:false} to the
readme ?

On Fri, Mar 21, 2014 at 10:50 PM, Rod Vagg notifications@github.com wrote:

I'm [image: 👍] on inherits() but [image: 👎] on debuglog. An
additional dependency just for debugging is a bit much. If you want it and
util.debuglog doesn't exist then just patch util before loading
reqdable-stream; that ought to be satisfactory for debugging situations.

Reply to this email directly or view it on GitHubhttps://github.com/isaacs/readable-stream/pull/83#issuecomment-38341105
.

-Calvin W. Metcalf

@rvagg

This comment has been minimized.

Show comment Hide comment
@rvagg

rvagg Mar 23, 2014

Owner

@feross @substack could you chime in here with browserify opinions please?

Owner

rvagg commented Mar 23, 2014

@feross @substack could you chime in here with browserify opinions please?

@feross

This comment has been minimized.

Show comment Hide comment
@feross

feross Mar 23, 2014

Contributor

Looks reasonable to me, minus the debuglog stuff. Once that's removed we can run browserify -r readable-stream | wc -c before and after this PR to see the size difference this makes.

Contributor

feross commented Mar 23, 2014

Looks reasonable to me, minus the debuglog stuff. Once that's removed we can run browserify -r readable-stream | wc -c before and after this PR to see the size difference this makes.

@calvinmetcalf

This comment has been minimized.

Show comment Hide comment
@calvinmetcalf

calvinmetcalf Mar 23, 2014

Member
167963 changes
184610 no changes

it would seem the bigger issue involves stream being imported

Member

calvinmetcalf commented Mar 23, 2014

167963 changes
184610 no changes

it would seem the bigger issue involves stream being imported

@calvinmetcalf

This comment has been minimized.

Show comment Hide comment
@calvinmetcalf

calvinmetcalf Mar 23, 2014

Member

removed the debuglog and got better numbers,

  • before (master branch) 184638
  • after (this branch) 167160
Member

calvinmetcalf commented Mar 23, 2014

removed the debuglog and got better numbers,

  • before (master branch) 184638
  • after (this branch) 167160
@feross

This comment has been minimized.

Show comment Hide comment
@feross

feross Mar 25, 2014

Contributor

Nice, that's a big win! Once we get rid of the 'stream' import this will be super small!

Contributor

feross commented Mar 25, 2014

Nice, that's a big win! Once we get rid of the 'stream' import this will be super small!

build/files.js
]
+ , debugLogReplacement = [
+ /var debug = util.debuglog\('stream'\);/

This comment has been minimized.

Show comment Hide comment
@rvagg

rvagg Mar 25, 2014

Owner

indent as per prevailing style

@rvagg

rvagg Mar 25, 2014

Owner

indent as per prevailing style

build/files.js
+ + '} else {\n'
+ + ' debug = function () {};\n'
+ + '}\n'
+ ], eventEmittterReplacement = [

This comment has been minimized.

Show comment Hide comment
@rvagg

rvagg Mar 25, 2014

Owner

2 newlines here to separate out the blocks

@rvagg

rvagg Mar 25, 2014

Owner

2 newlines here to separate out the blocks

build/files.js
]
+ , debugLogReplacement = [

This comment has been minimized.

Show comment Hide comment
@rvagg

rvagg Mar 25, 2014

Owner

newline pls

@rvagg

rvagg Mar 25, 2014

Owner

newline pls

@rvagg

This comment has been minimized.

Show comment Hide comment
@rvagg

rvagg Mar 25, 2014

Owner

cool beans, I'll set aside some time this week to sort all of this out and merge the various PRs that are pending so we can make proper progress!

Owner

rvagg commented Mar 25, 2014

cool beans, I'll set aside some time this week to sort all of this out and merge the various PRs that are pending so we can make proper progress!

@rvagg rvagg merged commit b586c98 into nodejs:master Mar 29, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment