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 `configurable: true` to browserwindow.loadSettingsJSON #16701

Merged
merged 2 commits into from Feb 8, 2018

Conversation

Projects
None yet
4 participants
@philipfweiss
Contributor

philipfweiss commented Feb 8, 2018

Description of the Change

Some packages, including Facebook's Nuclide, rely on monkey-patching browserWindow.loadSettingsJSON. Recent changes to the master branch of atom (the addition of the ConfigFile class / related code) changed the behavior of this property, adding a getter using Object.defineProperty.

With Object.defineProperty, configurable defaults to false, and it is impossible for packages to monkeypatch loadSettingsJSON. At the moment, this is a breaking change in Nuclide. Adding the configurable: true flag fixes this regression and allows us to redefine the getter.

Alternate Designs

Ideally we would like to fix the regression with only changes to Nuclide, but we could not find another way to monkeypatch browserWindow.loadSettingsJSON, without adding configurable: true to Object.defineProperty. If someone knows of an approach we would love to hear!

Why Should This Be In Core?

To fix a regression caused by a recent update to core.

Philip Weiss

@maxbrunsfeld maxbrunsfeld merged commit c47ed5e into master Feb 8, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxbrunsfeld maxbrunsfeld deleted the fb-pw-configurable-loadSettings branch Feb 8, 2018

@maxbrunsfeld

This comment has been minimized.

Contributor

maxbrunsfeld commented Feb 8, 2018

Some packages, including Facebook's Nuclide, rely on monkey-patching browserWindow.loadSettingsJSON.

At some point, it'd be good to talk about what the monkey patch is for and add some kind of public API to take its place, but I'm cool with this if it fixes an immediate problem.

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Feb 9, 2018

Should this be cherry picked to the 1.25-releases branch so it's included in the upcoming beta?

Object.defineProperty(this.browserWindow, 'loadSettingsJSON', {
get: () => JSON.stringify(Object.assign({
userSettings: this.atomApplication.configFile.get()
}, this.loadSettings))
})

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