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

Preserve custom properties passed to MenuItem constructor #7498

Merged
merged 7 commits into from Oct 11, 2016

Conversation

Projects
None yet
4 participants
@zeke
Member

zeke commented Oct 5, 2016

Fixes #7414

@zeke

This comment has been minimized.

Show comment
Hide comment
@zeke

zeke Oct 5, 2016

Member

Props to @jonathanGB for the code in this fix. #7414 (comment)

Member

zeke commented Oct 5, 2016

Props to @jonathanGB for the code in this fix. #7414 (comment)

@jonathanGB

This comment has been minimized.

Show comment
Hide comment
@jonathanGB

jonathanGB Oct 5, 2016

Glad it worked! :)

Glad it worked! :)

Show outdated Hide outdated lib/browser/api/menu-item.js
this.checked = options.checked
this.submenu = options.submenu
Object.assign(this, options)

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 5, 2016

Member

This is scary for a few reasons. Mostly because what happens if the user does something silly like pass in overrideProperty as a property on the options object.

This would causes the overrideProperty method to be overriden (oh how ironic). Can we blacklist that method from being overriden 👍

@MarshallOfSound

MarshallOfSound Oct 5, 2016

Member

This is scary for a few reasons. Mostly because what happens if the user does something silly like pass in overrideProperty as a property on the options object.

This would causes the overrideProperty method to be overriden (oh how ironic). Can we blacklist that method from being overriden 👍

This comment has been minimized.

@jonathanGB

jonathanGB Oct 6, 2016

@zeke What I propose is adding an "undesired" array of properties

var undesiredProperties = ['overrideProperty', 'overrideReadOnlyProperty' /* add more if necessary*/]

and just before calling .assign, delete the properties from options which are in that array, aka

undesiredProperties.forEach((undesiredProperty) => delete options[undesiredProperty])

Otherwise, an alternative would to either make methods like overrideProperty unwritable, or possibly put these customProperties inside another property like "custom" so passing properties like overrideProperty will not impact the existing method.

this.custom = Object.assign({}, getCustomProps(options))

In all cases, it'd be important to stay consistent between the handling of customProperties when using buildFromTemplate or through append

@jonathanGB

jonathanGB Oct 6, 2016

@zeke What I propose is adding an "undesired" array of properties

var undesiredProperties = ['overrideProperty', 'overrideReadOnlyProperty' /* add more if necessary*/]

and just before calling .assign, delete the properties from options which are in that array, aka

undesiredProperties.forEach((undesiredProperty) => delete options[undesiredProperty])

Otherwise, an alternative would to either make methods like overrideProperty unwritable, or possibly put these customProperties inside another property like "custom" so passing properties like overrideProperty will not impact the existing method.

this.custom = Object.assign({}, getCustomProps(options))

In all cases, it'd be important to stay consistent between the handling of customProperties when using buildFromTemplate or through append

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 6, 2016

Member

and just before calling .assign, delete the properties from options which are in that array, aka

You don't want to mutate an object the user provided, it will most likely annoy them 😆

Just set options to Object.assign({}, options) before doing this

@MarshallOfSound

MarshallOfSound Oct 6, 2016

Member

and just before calling .assign, delete the properties from options which are in that array, aka

You don't want to mutate an object the user provided, it will most likely annoy them 😆

Just set options to Object.assign({}, options) before doing this

This comment has been minimized.

@jonathanGB

jonathanGB Oct 6, 2016

very good point haha. I derped.

@jonathanGB

jonathanGB Oct 6, 2016

very good point haha. I derped.

This comment has been minimized.

@zeke

zeke Oct 6, 2016

Member

Good catch. Updated.

@zeke

zeke Oct 6, 2016

Member

Good catch. Updated.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Oct 10, 2016

Contributor

@zeke can this be removed now that we are doing this in the MenuItem constructor?

for (key in item) {
// Preserve extra fields specified by user
if (!menuItem.hasOwnProperty(key)) {
menuItem[key] = item[key]
}
}

Contributor

kevinsawicki commented Oct 10, 2016

@zeke can this be removed now that we are doing this in the MenuItem constructor?

for (key in item) {
// Preserve extra fields specified by user
if (!menuItem.hasOwnProperty(key)) {
menuItem[key] = item[key]
}
}

Show outdated Hide outdated lib/browser/api/menu-item.js
// Clone and sanitize the provided options
options = Object.assign({}, options)
delete options.overrideProperty
delete options.overrideReadOnlyProperty

This comment has been minimized.

@kevinsawicki

kevinsawicki Oct 10, 2016

Contributor

I don't think we want the options object to override any already defined instance properties or methods here, so maybe the hasOwnProperty guard that was previously used in Menu.buildFromTemplate is a better approach.

@kevinsawicki

kevinsawicki Oct 10, 2016

Contributor

I don't think we want the options object to override any already defined instance properties or methods here, so maybe the hasOwnProperty guard that was previously used in Menu.buildFromTemplate is a better approach.

This comment has been minimized.

@zeke

zeke Oct 10, 2016

Member

I tried that technique, but because hasOwnProperty only checks the object itself, and doesn't walk up the prototype chain, it was still possible to override methods like overrideProperty. I just updated it to use !(key in this) as a check instead.

@zeke

zeke Oct 10, 2016

Member

I tried that technique, but because hasOwnProperty only checks the object itself, and doesn't walk up the prototype chain, it was still possible to override methods like overrideProperty. I just updated it to use !(key in this) as a check instead.

Show outdated Hide outdated spec/api-menu-spec.js
assert.equal(menu.items[0].customProp, 'foo')
assert.equal(menu.items[0].submenu.items[0].label, 'item 1')
assert.equal(menu.items[0].submenu.items[0].customProp, 'bar')
assert.notEqual(typeof menu.items[0].submenu.items[0].overrideProperty, 'string')

This comment has been minimized.

@kevinsawicki

kevinsawicki Oct 10, 2016

Contributor

Can we assert that it is a function instead? That way if we ever rename that function the spec will fail so we know to update it.

@kevinsawicki

kevinsawicki Oct 10, 2016

Contributor

Can we assert that it is a function instead? That way if we ever rename that function the spec will fail so we know to update it.

This comment has been minimized.

@zeke

zeke Oct 10, 2016

Member

I wrote it this way to avoid coupling the test to the underlying implementation, but I see your point. 👍

@zeke

zeke Oct 10, 2016

Member

I wrote it this way to avoid coupling the test to the underlying implementation, but I see your point. 👍

zeke added some commits Oct 10, 2016

@kevinsawicki kevinsawicki self-assigned this Oct 11, 2016

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Oct 11, 2016

Contributor

Looks good to me, merge when 🚢

Contributor

kevinsawicki commented Oct 11, 2016

Looks good to me, merge when 🚢

@zeke zeke merged commit d4a8a64 into master Oct 11, 2016

9 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #4382775 succeeded in 66s
Details
electron-linux-ia32 Build #4382776 succeeded in 63s
Details
electron-linux-x64 Build #4382777 succeeded in 114s
Details
electron-mas-x64 Build #2590 succeeded in 9 min 8 sec
Details
electron-osx-x64 Build #2596 succeeded in 9 min 24 sec
Details
electron-win-ia32 Build #1685 succeeded in 8 min 1 sec
Details
electron-win-x64 Build #1660 succeeded in 8 min 0 sec
Details

@zeke zeke deleted the custom-props-in-menu-item-constructor branch Oct 11, 2016

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