Skip to content

feat(clone): option to not clone buffer, support custom properties#16

Closed
laurelnaiad wants to merge 1 commit intogulpjs:masterfrom
laurelnaiad:clone-buffer-optional2
Closed

feat(clone): option to not clone buffer, support custom properties#16
laurelnaiad wants to merge 1 commit intogulpjs:masterfrom
laurelnaiad:clone-buffer-optional2

Conversation

@laurelnaiad
Copy link
Copy Markdown
Contributor

option { contents: false } causes the clone to reference-copy buffer contents.

Custom properties that exist in the original file are deep-cloned to the new File.

option `{ contents: false }` causes the clone to reference-copy buffer contents.

Custom properties that exist in the original file are deep-cloned to the new File.
Comment thread index.js
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hasOwnProperty is unnecessary as Object.keys() doesn't include properties of the prototype chain.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hasOwnProperty is redundant, where does _contents come from? That isn't a standard attribute

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@contra _contents is an internal property (L113-L123) because contents is virtual.

@yocontra
Copy link
Copy Markdown
Member

Sorry for letting this sit for so long, I was traveling. I will look at this now

Comment thread index.js
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't all attributes follow the same code path? If opt[k] === false then reference, else clone?

@popomore
Copy link
Copy Markdown
Contributor

popomore commented Jul 4, 2014

How about this PR? I want this feature.

@yocontra
Copy link
Copy Markdown
Member

yocontra commented Jul 5, 2014

In the woods with bad internet connection right now. I will review when I'm back in town

@popomore
Copy link
Copy Markdown
Contributor

popomore commented Jul 5, 2014

All right, have a good travel—
Sent from Mailbox for iPhone

On Sat, Jul 5, 2014 at 1:31 PM, Eric Schoffstall notifications@github.com
wrote:

In the woods with bad internet connection right now. I will review when I'm back in town

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

@popomore
Copy link
Copy Markdown
Contributor

@contra Are you coming back, do you have time to review this pr?

@yocontra
Copy link
Copy Markdown
Member

I never got a response to the code comments I left :/

@popomore
Copy link
Copy Markdown
Contributor

Or you can review my pr #26 as you like, I just want to support this feature.

@vweevers
Copy link
Copy Markdown
Contributor

How about a PR for just the custom properties feature? Because the holdup here is about reference-copying and @stu-salsbury not responding.

@vweevers
Copy link
Copy Markdown
Contributor

@contra If you agree, I'll create that PR from stu-salsbury's code (without the reference options). (@popomore: yours doesn't do a deep clone)

@yocontra
Copy link
Copy Markdown
Member

I'll take a wholesome PR - code style matches, tests, etc. any day and merge it quickly as long as it looks good

@vweevers Send that PR

@popomore
Copy link
Copy Markdown
Contributor

Good, thx @vweevers . If you have no time, I will update my PR.

@popomore
Copy link
Copy Markdown
Contributor

deep clone has been implemented, need contents option?

@yocontra
Copy link
Copy Markdown
Member

@popomore Yep - btw could you try to use a lower level clone (like the node-v8-clone module) instead of lodash

@popomore
Copy link
Copy Markdown
Contributor

Have a try, but I just want pure javascript, especially for Windows.

@yocontra
Copy link
Copy Markdown
Member

@popomore Native modules should work fine on windows, you can make it an optionalDependency in the package.json and fallback to lodash if it fails to install

popomore added a commit to popomore/vinyl that referenced this pull request Aug 28, 2014
popomore added a commit to popomore/vinyl that referenced this pull request Aug 28, 2014
@yocontra
Copy link
Copy Markdown
Member

Fixed by #32

@yocontra yocontra closed this Aug 29, 2014
phated pushed a commit that referenced this pull request Sep 27, 2016
phated pushed a commit that referenced this pull request Sep 27, 2016
phated pushed a commit that referenced this pull request Sep 27, 2016
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