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

improve clone #32

Merged
merged 7 commits into from
Aug 29, 2014
Merged

improve clone #32

merged 7 commits into from
Aug 29, 2014

Conversation

popomore
Copy link
Contributor

  • clone history
  • option for deep clone, default: false
  • option for cloneBuffer, default: true
  • use node-v8-clone for performance, fallback to lodash
  • add jshintrc to check code style

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f76a921 on popomore:clone into c801d3d on wearefractal:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 7724121 on popomore:clone into c801d3d on wearefractal:master.

var _ = require('lodash');
var cloneDeep = _.cloneDeep;

var cloneBuffer = require('./lib/cloneBuffer');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this needed with node-v8-clone?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cloneBuffer just copy buffer, don't require clone.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 037e830 on popomore:clone into c801d3d on wearefractal:master.

yocontra added a commit that referenced this pull request Aug 29, 2014
@yocontra yocontra merged commit 76165e0 into gulpjs:master Aug 29, 2014
@popomore popomore deleted the clone branch August 29, 2014 04:04
}
}, this);

clone.contents = this.isBuffer() ? cloneBuffer(this.contents) : this.contents;
clone.stat = this.stat ? cloneStats(this.stat) : null;
file.contents = opt.contents && this.isBuffer() ? cloneBuffer(this.contents) : this.contents;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

want to send another pull request adding in the stream dereferencing?

@sindresorhus
Copy link

@contra using a native dep is not a good idea for a myriad of reasons:

  • It will fail on most computers as few have the required toolchain, especially on Windows. Leaving this mostly moot in benefits, but high in downsides:
  • The gyp error makes people believe the install failed: https://twitter.com/ArnaudHuc/status/505326409884397569
  • node-v8-clone takes almost 2 minutes to install (because it downloads node), on my fast OS X 10.9 machine with the required toolchain to build.

@popomore
Copy link
Contributor Author

Yeah, so it's an option dep, it will fallback to lodash, other idea for clone performance?—
Sent from Mailbox for iPhone

On Fri, Aug 29, 2014 at 8:36 PM, Sindre Sorhus notifications@github.com
wrote:

@contra using a native dep is not a good idea for a myriad of reasons:

- node-v8-clone takes almost 2 minutes to install (because it downloads node).

Reply to this email directly or view it on GitHub:
#32 (comment)

@yocontra
Copy link
Member

@sindresorhus So it's outputting the gyp error even though it is listed as an optionalDependency? :(

@sindresorhus
Copy link

@popomore ⬆️ I think you went with a native dep too fast without looking into why it's slow.

@contra Yes

@popomore
Copy link
Contributor Author

I like pure javascript too, we should find whether it's slow exactly and why?

@yocontra
Copy link
Member

@popomore Just an FYI the history PR and this PR conflicted, history (an array) was not being deep cloned unless specifically told to - this is a special case like stat that always needs to be deep. I patched this already (0.4.1).

I'm okay with going pure JS, I wish native deps weren't such a hassle because the v8 clone is an order of magnitude faster. Is there anything faster than JS?

@popomore
Copy link
Contributor Author

@contra ok, that's my fault.

phated pushed a commit that referenced this pull request Sep 27, 2016
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 this pull request may close these issues.

4 participants