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

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

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

laurelnaiad
Copy link
Contributor

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

Custom properties that exist in the original file are reference-copied.

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

Custom properties that exist in the original file are reference-copied.
Add documentation for the new `contents: false` option on the `clone` function.
var clonedStat = this.stat ? cloneStats(this.stat) : null;

return new File({
var newFile = new File({
cwd: this.cwd,
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to specify these since we are cloning all properties anyways?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I worked around the inbuilt properties in my loop so as to use the constructor where I could, just in case the constructor ends up doing something more interesting in the future. Subject to your other comment, I guess that all properties could be deep cloned, and then I can test whether or not to call cloneBuffer afterward based on the flag. On the deep cloning, do you have a favorite method? How about https://github.com/pvorb/node-clone?

Copy link
Member

Choose a reason for hiding this comment

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

@stu-salsbury Not sure which is more performant, but I'm sure lodash has a pretty good deep 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.

Lodash it is. My preference anyway. I was looking for something small and targeted, but it's not like this is running in a browser, thankfully.

Copy link
Member

Choose a reason for hiding this comment

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

@stu-salsbury You can install only certain pieces of lodash (like http://npmjs.org/lodash.clone)

All properties of the original file are deep-cloned into the new file, with the exception of contents and stat, which both require special handling.
@laurelnaiad
Copy link
Contributor Author

I had to keep the manual cloning of stat, since it's not a POJSO.

@laurelnaiad
Copy link
Contributor Author

If you want me to squash the commits and do a new PR once this is all settled, I'm cool with that.

Remove full lodash dependency.  Depend only on lodash.cloneDeep.
@laurelnaiad
Copy link
Contributor Author

Closing in favor of #16.

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.

None yet

2 participants