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

Add comment about combined _.defaults and _.extend #4088

Closed
wants to merge 6 commits into from
Closed

Add comment about combined _.defaults and _.extend #4088

wants to merge 6 commits into from

Conversation

unclechu
Copy link
Contributor

No description provided.

Copy link
Collaborator

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Please undo changes to everything but backbone.js. All of the others will be built later.

@@ -400,7 +400,11 @@
if (options.collection) this.collection = options.collection;
if (options.parse) attrs = this.parse(attrs, options) || {};
var defaults = _.result(this, 'defaults');

// Additional `_.defaults()` wrap after `_.extend()` makes sense
// when `attrs` has a property explicitly set to `undefined`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's link to #3842.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than "makes sense" can you explain the behavior that it actually avoids?

@unclechu
Copy link
Contributor Author

unclechu commented Oct 12, 2016

@jridgewell it's done. Is it okay that I made this revert commit? I mean if you do squash merge at the end it wont matter.

@jridgewell
Copy link
Collaborator

No, we can squash merge it. Still need the link to #3842. And someone else should look over the wording. @captbaritone? 😉

@unclechu
Copy link
Contributor Author

@jridgewell

Still need the link to #3842

Do you mean I should add #3842 to the commit message?

@jridgewell
Copy link
Collaborator

To the comment, not the commit message.


// Additional `_.defaults()` wrap after `_.extend()` makes sense
// when `attrs` has a property explicitly set to `undefined`.
// Discussion: https://github.com/jashkenas/backbone/issues/3842
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just use GitHub's issue shorthand #1234. You could use this as a model: https://github.com/jashkenas/backbone/blob/master/backbone.js#L1158-L1159

@unclechu
Copy link
Contributor Author

@jrburke how about now?

@captbaritone
Copy link
Contributor

Rather than "makes sense" can you explain the behavior that it actually avoids?

@unclechu
Copy link
Contributor Author

@captbaritone I added line about it helps to avoid conflicts with Object.prototype properties.

@captbaritone
Copy link
Contributor

I'm not familiar with this issue, and those who are reading this comment probably won't be either. In what way does it "make sense"? Does it preserve attributes which have been explicitly set to undefined? Does it overwrite them?

@jridgewell
Copy link
Collaborator

It has two steps.

  1. We have to create an object that contains all properties, attrs taking precedence over defaults. We clobber prototype inheritance here on purpose.
    • _.extend({}, defaults, attrs)
  2. Anything in Step 1 that is undefined should be overridden with its defaults value. Since we clobbered prototype inheritance, we know for certain that if something is undefined, it was set specifically to undefined in attrs. (Or, if it was set in defaults, it will still be undefined after this step)
    • _.defaults(_, defaults)

jgonggrijp pushed a commit to jgonggrijp/backbone that referenced this pull request Jul 19, 2023
jgonggrijp added a commit to jgonggrijp/backbone that referenced this pull request Jul 19, 2023
@jgonggrijp
Copy link
Collaborator

Superseded by #4267.

@jgonggrijp jgonggrijp closed this Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants