From 7c3fe7dba42a9d99a0a86877a43aa791dd6456d9 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 23 Aug 2017 17:43:47 -0700 Subject: [PATCH 01/28] Replace pathwatcher w/ bundled watcher to catch created & rename events --- src/config.coffee | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/config.coffee b/src/config.coffee index f0628ffee..bb506f5f4 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -4,7 +4,7 @@ fs = require 'fs-plus' CSON = require 'season' path = require 'path' async = require 'async' -pathWatcher = require 'pathwatcher' +{watchPath} = require './path-watcher' { getValueAtKeyPath, setValueAtKeyPath, deleteValueAtKeyPath, pushKeyPath, splitKeyPath, @@ -880,8 +880,9 @@ class Config return if @shouldNotAccessFileSystem() try - @watchSubscription ?= pathWatcher.watch @configFilePath, (eventType) => - @requestLoad() if eventType is 'change' and @watchSubscription? + @watchSubscription ?= watchPath @configFilePath, {}, (events) => + for {action} in events + @requestLoad() if action in ['created', 'modified', 'renamed'] and @watchSubscription? catch error @notifyFailure """ Unable to watch path: `#{path.basename(@configFilePath)}`. Make sure you have permissions to @@ -891,7 +892,7 @@ class Config """ unobserveUserConfig: -> - @watchSubscription?.close() + @watchSubscription?.dispose() @watchSubscription = null notifyFailure: (errorMessage, detail) -> From 1dc5dec816761861594a4a0f3e3810061a0a4c39 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 25 Aug 2017 19:58:51 -0700 Subject: [PATCH 02/28] WIP fix broken tests --- spec/config-spec.coffee | 27 +++++++++++++++++++++------ src/config.coffee | 17 +++++++++++++---- 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/spec/config-spec.coffee b/spec/config-spec.coffee index 552f44bf9..be8e8bdfd 100644 --- a/spec/config-spec.coffee +++ b/spec/config-spec.coffee @@ -899,12 +899,15 @@ describe "Config", -> previousSetTimeoutCallCount = setTimeout.callCount runs -> fs.writeFileSync(atom.config.configFilePath, data) - waitsFor "debounced config file load", -> - setTimeout.callCount > previousSetTimeoutCallCount + # waitsFor "debounced config file load", -> + # setTimeout.callCount > previousSetTimeoutCallCount + waitsFor "file written", -> + fs.readFileSync(atom.config.configFilePath, 'utf8') is data runs -> advanceClock(1000) beforeEach -> + console.log 'beforeEach' atom.config.setSchema 'foo', type: 'object' properties: @@ -930,16 +933,28 @@ describe "Config", -> scoped: true """ atom.config.loadUserConfig() - atom.config.observeUserConfig() - updatedHandler = jasmine.createSpy("updatedHandler") - atom.config.onDidChange updatedHandler + + console.log 'observeUserConfig promise', atom.config.observeUserConfig() + waitsForPromise -> atom.config.observeUserConfig() + + runs -> + updatedHandler = jasmine.createSpy("updatedHandler") + atom.config.onDidChange updatedHandler afterEach -> + # WHY IS THIS NOT RUNNING? + console.log 'afterEach' atom.config.unobserveUserConfig() fs.removeSync(dotAtomPath) describe "when the config file changes to contain valid cson", -> - it "updates the config data", -> + afterEach -> + # WHY IS THIS NOT RUNNING? + console.log 'afterEach' + atom.config.unobserveUserConfig() + fs.removeSync(dotAtomPath) + + fit "updates the config data", -> writeConfigFile("foo: { bar: 'quux', baz: 'bar'}") waitsFor 'update event', -> updatedHandler.callCount > 0 runs -> diff --git a/src/config.coffee b/src/config.coffee index bb506f5f4..86352c189 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -849,6 +849,7 @@ class Config loadUserConfig: -> return if @shouldNotAccessFileSystem() + console.log 'loadUserConfig' try unless fs.existsSync(@configFilePath) fs.makeTreeSync(path.dirname(@configFilePath)) @@ -880,9 +881,14 @@ class Config return if @shouldNotAccessFileSystem() try - @watchSubscription ?= watchPath @configFilePath, {}, (events) => + console.trace 'create watch subscription', @watchSubscriptionPromise + @watchSubscriptionPromise ?= watchPath @configFilePath, {}, (events) => + console.log events for {action} in events - @requestLoad() if action in ['created', 'modified', 'renamed'] and @watchSubscription? + console.log action, @watchSubscriptionPromise? + if action in ['created', 'modified', 'renamed'] and @watchSubscriptionPromise? + console.warn 'request load' + @requestLoad() catch error @notifyFailure """ Unable to watch path: `#{path.basename(@configFilePath)}`. Make sure you have permissions to @@ -891,9 +897,12 @@ class Config [watches]:https://github.com/atom/atom/blob/master/docs/build-instructions/linux.md#typeerror-unable-to-watch-path """ + @watchSubscriptionPromise + unobserveUserConfig: -> - @watchSubscription?.dispose() - @watchSubscription = null + @watchSubscriptionPromise?.then((watcher) => watcher?.dispose()) + @watchSubscriptionPromise = null + console.log 'unobserve' notifyFailure: (errorMessage, detail) -> @notificationManager?.addError(errorMessage, {detail, dismissable: true}) From b494d0fb9ee9caeb8cb70c4bb22bfd290b41353b Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 25 Aug 2017 17:18:34 -0700 Subject: [PATCH 03/28] WIP don't overwrite config file if it exists Depends on https://github.com/atom/season/pull/22 --- src/config.coffee | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/config.coffee b/src/config.coffee index 86352c189..389350ef9 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -851,6 +851,8 @@ class Config console.log 'loadUserConfig' try + # fs.makeTreeSync(path.dirname(@configFilePath)) + # CSON.writeFileSync(@configFilePath, {flag: 'x'}, {}) # fails if file exists unless fs.existsSync(@configFilePath) fs.makeTreeSync(path.dirname(@configFilePath)) CSON.writeFileSync(@configFilePath, {}) From a0766d9b69d3794154591a3cebf027c813729e78 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 25 Aug 2017 20:29:07 -0700 Subject: [PATCH 04/28] Ensure set/unset operations take place after user's config is loaded --- src/config.coffee | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/config.coffee b/src/config.coffee index 389350ef9..6c47caf88 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -399,6 +399,8 @@ class Config # Created during initialization, available as `atom.config` constructor: ({@notificationManager, @enablePersistence}={}) -> + @settingsLoaded = false + @pendingOperations = [] @clear() initialize: ({@configDirPath, @resourcePath, projectHomeSchema}) -> @@ -646,6 +648,10 @@ class Config # * `true` if the value was set. # * `false` if the value was not able to be coerced to the type specified in the setting's schema. set: -> + unless @settingsLoaded + @pendingOperations.push(() => @set.apply(@, arguments)) + return + [keyPath, value, options] = arguments scopeSelector = options?.scopeSelector source = options?.source @@ -677,6 +683,10 @@ class Config # * `scopeSelector` (optional) {String}. See {::set} # * `source` (optional) {String}. See {::set} unset: (keyPath, options) -> + unless @settingsLoaded + @pendingOperations.push(() => @unset.apply(@, arguments)) + return + {scopeSelector, source} = options ? {} source ?= @getUserConfigPath() @@ -944,7 +954,12 @@ class Config @transact => @settings = {} + @settingsLoaded = true @set(key, value, save: false) for key, value of newSettings + if @pendingOperations.length + op() for op in @pendingOperations + @debouncedSave() + @pendingOperations = [] return getRawValue: (keyPath, options) -> From 494cb7ea4b2f5e9e243971ef74c8521a689268fe Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 25 Aug 2017 20:32:17 -0700 Subject: [PATCH 05/28] WIP Add test for ensuring that set/unset operations take place after load --- spec/config-spec.coffee | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/spec/config-spec.coffee b/spec/config-spec.coffee index be8e8bdfd..495ce031e 100644 --- a/spec/config-spec.coffee +++ b/spec/config-spec.coffee @@ -1756,3 +1756,34 @@ describe "Config", -> expect(atom.config.set('foo.bar.str_options', 'One')).toBe false expect(atom.config.get('foo.bar.str_options')).toEqual 'two' + describe "when .set/.unset is called prior to .loadUserConfig", -> + console.log 'this test' + beforeEach -> + fs.writeFileSync config.configFilePath, """ + '*': + foo: + bar: 'baz' + do: + ray: 'me' + """ + + it "ensures that all settings are loaded correctly", -> + console.log 'test start' + config.unset('foo.bar') + expect(config.save).not.toHaveBeenCalled() + config.set('foo.qux', 'boo') + expect(config.save).not.toHaveBeenCalled() + expect(config.get('foo.qux')).toBeUndefined() + expect(config.get('do.ray')).toBeUndefined() + + console.log 'loadUserConfig' + config.loadUserConfig() + + waitsFor -> config.get('foo.bar') is undefined + runs -> + expect(config.save).toHaveBeenCalled() + expect(config.get('foo.bar')).toBeUndefined() + expect(config.get('foo.qux')).toBe('boo') + expect(config.get('do.ray')).toBe('me') + + console.log 'end test' From a79ee746d144d05516d2430af3c1cbdb4f28050b Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 11 Sep 2017 09:20:44 -0400 Subject: [PATCH 06/28] :arrow_up: season --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 80290b47a..f42f96c24 100644 --- a/package.json +++ b/package.json @@ -64,7 +64,7 @@ "scandal": "^3.1.0", "scoped-property-store": "^0.17.0", "scrollbar-style": "^3.2", - "season": "^6.0.0", + "season": "^6.0.1", "semver": "^4.3.3", "service-hub": "^0.7.4", "sinon": "1.17.4", From 046ff87f0167814ec08576a371b90bd42af969c9 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 10:12:38 -0400 Subject: [PATCH 07/28] Trick the specs into seeing the settings as loaded --- spec/config-spec.coffee | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/config-spec.coffee b/spec/config-spec.coffee index 495ce031e..e1cd754d9 100644 --- a/spec/config-spec.coffee +++ b/spec/config-spec.coffee @@ -13,6 +13,7 @@ describe "Config", -> dotAtomPath = temp.path('atom-spec-config') atom.config.configDirPath = dotAtomPath atom.config.enablePersistence = true + atom.config.settingsLoaded = true atom.config.configFilePath = path.join(atom.config.configDirPath, "atom.config.cson") afterEach -> From 21e220cd5fb1cd0250e6e34d9024e1dbf68b053f Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 10:13:05 -0400 Subject: [PATCH 08/28] Stub the correct fs method --- spec/config-spec.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/config-spec.coffee b/spec/config-spec.coffee index e1cd754d9..cb4b19113 100644 --- a/spec/config-spec.coffee +++ b/spec/config-spec.coffee @@ -878,7 +878,7 @@ describe "Config", -> beforeEach -> atom.notifications.onDidAddNotification addErrorHandler = jasmine.createSpy() - spyOn(fs, "existsSync").andCallFake -> + spyOn(fs, "makeTreeSync").andCallFake -> error = new Error() error.code = 'EPERM' throw error From fa0bd1e049fe39e752e4e8cc64ff91ba5aae2559 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 10:15:55 -0400 Subject: [PATCH 09/28] Use a real clock and artificial file mtimes to work with nsfw --- spec/config-spec.coffee | 57 +++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/spec/config-spec.coffee b/spec/config-spec.coffee index cb4b19113..b33b9226b 100644 --- a/spec/config-spec.coffee +++ b/spec/config-spec.coffee @@ -896,19 +896,15 @@ describe "Config", -> describe ".observeUserConfig()", -> updatedHandler = null - writeConfigFile = (data) -> - previousSetTimeoutCallCount = setTimeout.callCount - runs -> - fs.writeFileSync(atom.config.configFilePath, data) - # waitsFor "debounced config file load", -> - # setTimeout.callCount > previousSetTimeoutCallCount - waitsFor "file written", -> - fs.readFileSync(atom.config.configFilePath, 'utf8') is data - runs -> - advanceClock(1000) + writeConfigFile = (data, secondsInFuture = 0) -> + fs.writeFileSync(atom.config.configFilePath, data) + + future = (Date.now() / 1000) + secondsInFuture + fs.utimesSync(atom.config.configFilePath, future, future) beforeEach -> - console.log 'beforeEach' + jasmine.useRealClock() + atom.config.setSchema 'foo', type: 'object' properties: @@ -924,7 +920,7 @@ describe "Config", -> default: 12 expect(fs.existsSync(atom.config.configDirPath)).toBeFalsy() - fs.writeFileSync atom.config.configFilePath, """ + writeConfigFile """ '*': foo: bar: 'baz' @@ -955,9 +951,11 @@ describe "Config", -> atom.config.unobserveUserConfig() fs.removeSync(dotAtomPath) - fit "updates the config data", -> - writeConfigFile("foo: { bar: 'quux', baz: 'bar'}") + it "updates the config data", -> + writeConfigFile "foo: { bar: 'quux', baz: 'bar'}", 2 + waitsFor 'update event', -> updatedHandler.callCount > 0 + runs -> expect(atom.config.get('foo.bar')).toBe 'quux' expect(atom.config.get('foo.baz')).toBe 'bar' @@ -965,7 +963,7 @@ describe "Config", -> it "does not fire a change event for paths that did not change", -> atom.config.onDidChange 'foo.bar', noChangeSpy = jasmine.createSpy() - writeConfigFile("foo: { bar: 'baz', baz: 'ok'}") + writeConfigFile "foo: { bar: 'baz', baz: 'ok'}", 2 waitsFor 'update event', -> updatedHandler.callCount > 0 runs -> @@ -980,7 +978,8 @@ describe "Config", -> items: type: 'string' - writeConfigFile("foo: { bar: ['baz', 'ok']}") + updatedHandler.reset() + writeConfigFile "foo: { bar: ['baz', 'ok']}", 4 waitsFor 'update event', -> updatedHandler.callCount > 0 runs -> updatedHandler.reset() @@ -988,7 +987,7 @@ describe "Config", -> noChangeSpy = jasmine.createSpy() atom.config.onDidChange('foo.bar', noChangeSpy) - writeConfigFile("foo: { bar: ['baz', 'ok'], baz: 'another'}") + writeConfigFile "foo: { bar: ['baz', 'ok'], baz: 'another'}", 2 waitsFor 'update event', -> updatedHandler.callCount > 0 runs -> @@ -1005,7 +1004,7 @@ describe "Config", -> '*': foo: scoped: false - """ + """, 2 waitsFor 'update event', -> updatedHandler.callCount > 0 runs -> @@ -1023,7 +1022,7 @@ describe "Config", -> '.source.ruby': foo: scoped: true - """ + """, 2 waitsFor 'update event', -> updatedHandler.callCount > 0 runs -> @@ -1033,7 +1032,7 @@ describe "Config", -> describe "when the config file changes to omit a setting with a default", -> it "resets the setting back to the default", -> - writeConfigFile("foo: { baz: 'new'}") + writeConfigFile "foo: { baz: 'new'}", 2 waitsFor 'update event', -> updatedHandler.callCount > 0 runs -> expect(atom.config.get('foo.bar')).toBe 'def' @@ -1041,20 +1040,20 @@ describe "Config", -> describe "when the config file changes to be empty", -> beforeEach -> - writeConfigFile("") + updatedHandler.reset() + writeConfigFile "", 4 waitsFor 'update event', -> updatedHandler.callCount > 0 it "resets all settings back to the defaults", -> expect(updatedHandler.callCount).toBe 1 expect(atom.config.get('foo.bar')).toBe 'def' atom.config.set("hair", "blonde") # trigger a save - advanceClock(500) - expect(atom.config.save).toHaveBeenCalled() + waitsFor 'save', -> atom.config.save.callCount > 0 describe "when the config file subsequently changes again to contain configuration", -> beforeEach -> updatedHandler.reset() - writeConfigFile("foo: bar: 'newVal'") + writeConfigFile "foo: bar: 'newVal'", 2 waitsFor 'update event', -> updatedHandler.callCount > 0 it "sets the setting to the value specified in the config file", -> @@ -1064,24 +1063,25 @@ describe "Config", -> addErrorHandler = null beforeEach -> atom.notifications.onDidAddNotification addErrorHandler = jasmine.createSpy() - writeConfigFile("}}}") + writeConfigFile "}}}", 4 waitsFor "error to be logged", -> addErrorHandler.callCount > 0 it "logs a warning and does not update config data", -> expect(updatedHandler.callCount).toBe 0 expect(atom.config.get('foo.bar')).toBe 'baz' + atom.config.set("hair", "blonde") # trigger a save expect(atom.config.save).not.toHaveBeenCalled() describe "when the config file subsequently changes again to contain valid cson", -> beforeEach -> - writeConfigFile("foo: bar: 'newVal'") + updatedHandler.reset() + writeConfigFile "foo: bar: 'newVal'", 6 waitsFor 'update event', -> updatedHandler.callCount > 0 it "updates the config data and resumes saving", -> atom.config.set("hair", "blonde") - advanceClock(500) - expect(atom.config.save).toHaveBeenCalled() + waitsFor 'save', -> atom.config.save.callCount > 0 describe ".initializeConfigDirectory()", -> beforeEach -> @@ -1757,6 +1757,7 @@ describe "Config", -> expect(atom.config.set('foo.bar.str_options', 'One')).toBe false expect(atom.config.get('foo.bar.str_options')).toEqual 'two' + describe "when .set/.unset is called prior to .loadUserConfig", -> console.log 'this test' beforeEach -> From bf121eab722c47c93c3daa09a000309d60188d27 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 10:16:25 -0400 Subject: [PATCH 10/28] Remove some diagnostics --- spec/config-spec.coffee | 9 --------- 1 file changed, 9 deletions(-) diff --git a/spec/config-spec.coffee b/spec/config-spec.coffee index b33b9226b..8088c96e3 100644 --- a/spec/config-spec.coffee +++ b/spec/config-spec.coffee @@ -931,7 +931,6 @@ describe "Config", -> """ atom.config.loadUserConfig() - console.log 'observeUserConfig promise', atom.config.observeUserConfig() waitsForPromise -> atom.config.observeUserConfig() runs -> @@ -939,17 +938,10 @@ describe "Config", -> atom.config.onDidChange updatedHandler afterEach -> - # WHY IS THIS NOT RUNNING? - console.log 'afterEach' atom.config.unobserveUserConfig() fs.removeSync(dotAtomPath) describe "when the config file changes to contain valid cson", -> - afterEach -> - # WHY IS THIS NOT RUNNING? - console.log 'afterEach' - atom.config.unobserveUserConfig() - fs.removeSync(dotAtomPath) it "updates the config data", -> writeConfigFile "foo: { bar: 'quux', baz: 'bar'}", 2 @@ -1759,7 +1751,6 @@ describe "Config", -> expect(atom.config.get('foo.bar.str_options')).toEqual 'two' describe "when .set/.unset is called prior to .loadUserConfig", -> - console.log 'this test' beforeEach -> fs.writeFileSync config.configFilePath, """ '*': From adb032adf11895a6475e9e4c9d15f128d3d14080 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 10:16:50 -0400 Subject: [PATCH 11/28] Some cosmetic coffeescript changes --- spec/config-spec.coffee | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/config-spec.coffee b/spec/config-spec.coffee index 8088c96e3..1a90a25e7 100644 --- a/spec/config-spec.coffee +++ b/spec/config-spec.coffee @@ -934,7 +934,7 @@ describe "Config", -> waitsForPromise -> atom.config.observeUserConfig() runs -> - updatedHandler = jasmine.createSpy("updatedHandler") + updatedHandler = jasmine.createSpy "updatedHandler" atom.config.onDidChange updatedHandler afterEach -> @@ -953,7 +953,7 @@ describe "Config", -> expect(atom.config.get('foo.baz')).toBe 'bar' it "does not fire a change event for paths that did not change", -> - atom.config.onDidChange 'foo.bar', noChangeSpy = jasmine.createSpy() + atom.config.onDidChange 'foo.bar', noChangeSpy = jasmine.createSpy "unchanged" writeConfigFile "foo: { bar: 'baz', baz: 'ok'}", 2 waitsFor 'update event', -> updatedHandler.callCount > 0 @@ -976,7 +976,7 @@ describe "Config", -> runs -> updatedHandler.reset() it "does not fire a change event for paths that did not change", -> - noChangeSpy = jasmine.createSpy() + noChangeSpy = jasmine.createSpy "unchanged" atom.config.onDidChange('foo.bar', noChangeSpy) writeConfigFile "foo: { bar: ['baz', 'ok'], baz: 'another'}", 2 @@ -1004,7 +1004,7 @@ describe "Config", -> expect(atom.config.get('foo.scoped', scope: ['.source.ruby'])).toBe false it "does not fire a change event for paths that did not change", -> - noChangeSpy = jasmine.createSpy() + noChangeSpy = jasmine.createSpy "no change" atom.config.onDidChange('foo.scoped', scope: ['.source.ruby'], noChangeSpy) writeConfigFile """ @@ -1054,7 +1054,7 @@ describe "Config", -> describe "when the config file changes to contain invalid cson", -> addErrorHandler = null beforeEach -> - atom.notifications.onDidAddNotification addErrorHandler = jasmine.createSpy() + atom.notifications.onDidAddNotification addErrorHandler = jasmine.createSpy "error handler" writeConfigFile "}}}", 4 waitsFor "error to be logged", -> addErrorHandler.callCount > 0 From e9588c8fae036e63ca825fbfa7811c9c3acaafd0 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 10:18:09 -0400 Subject: [PATCH 12/28] Bring the new spec up to date --- spec/config-spec.coffee | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/spec/config-spec.coffee b/spec/config-spec.coffee index 1a90a25e7..14fbd64d0 100644 --- a/spec/config-spec.coffee +++ b/spec/config-spec.coffee @@ -1752,7 +1752,9 @@ describe "Config", -> describe "when .set/.unset is called prior to .loadUserConfig", -> beforeEach -> - fs.writeFileSync config.configFilePath, """ + atom.config.settingsLoaded = false + + fs.writeFileSync atom.config.configFilePath, """ '*': foo: bar: 'baz' @@ -1761,22 +1763,19 @@ describe "Config", -> """ it "ensures that all settings are loaded correctly", -> - console.log 'test start' - config.unset('foo.bar') - expect(config.save).not.toHaveBeenCalled() - config.set('foo.qux', 'boo') - expect(config.save).not.toHaveBeenCalled() - expect(config.get('foo.qux')).toBeUndefined() - expect(config.get('do.ray')).toBeUndefined() + atom.config.unset('foo.bar') + expect(atom.config.save).not.toHaveBeenCalled() + atom.config.set('foo.qux', 'boo') + expect(atom.config.save).not.toHaveBeenCalled() + expect(atom.config.get('foo.qux')).toBeUndefined() + expect(atom.config.get('do.ray')).toBeUndefined() - console.log 'loadUserConfig' - config.loadUserConfig() + atom.config.loadUserConfig() + advanceClock 100 + + waitsFor -> atom.config.save.callCount > 0 - waitsFor -> config.get('foo.bar') is undefined runs -> - expect(config.save).toHaveBeenCalled() - expect(config.get('foo.bar')).toBeUndefined() - expect(config.get('foo.qux')).toBe('boo') - expect(config.get('do.ray')).toBe('me') - - console.log 'end test' + expect(atom.config.get('foo.bar')).toBeUndefined() + expect(atom.config.get('foo.qux')).toBe('boo') + expect(atom.config.get('do.ray')).toBe('me') From 8b94ed95586c307f14a942509f3acd595b007a71 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 10:18:47 -0400 Subject: [PATCH 13/28] "arguments" is overwritten by closure arguments --- src/config.coffee | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/config.coffee b/src/config.coffee index 6c47caf88..e23724885 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -648,11 +648,12 @@ class Config # * `true` if the value was set. # * `false` if the value was not able to be coerced to the type specified in the setting's schema. set: -> + [keyPath, value, options] = arguments + unless @settingsLoaded - @pendingOperations.push(() => @set.apply(@, arguments)) + @pendingOperations.push () => @set.call(this, keyPath, value, options) return - [keyPath, value, options] = arguments scopeSelector = options?.scopeSelector source = options?.source shouldSave = options?.save ? true @@ -684,7 +685,7 @@ class Config # * `source` (optional) {String}. See {::set} unset: (keyPath, options) -> unless @settingsLoaded - @pendingOperations.push(() => @unset.apply(@, arguments)) + @pendingOperations.push () => @unset.call(this, keyPath, options) return {scopeSelector, source} = options ? {} From b45fb2e918d5c2b5a519bb743c078d8ad3821870 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 10:19:38 -0400 Subject: [PATCH 14/28] Use the wx flag to atomically create a config file if it doesn't exist --- src/config.coffee | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/config.coffee b/src/config.coffee index e23724885..0d95ab2ef 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -862,11 +862,8 @@ class Config console.log 'loadUserConfig' try - # fs.makeTreeSync(path.dirname(@configFilePath)) - # CSON.writeFileSync(@configFilePath, {flag: 'x'}, {}) # fails if file exists - unless fs.existsSync(@configFilePath) - fs.makeTreeSync(path.dirname(@configFilePath)) - CSON.writeFileSync(@configFilePath, {}) + fs.makeTreeSync(path.dirname(@configFilePath)) + CSON.writeFileSync(@configFilePath, {}, {flag: 'wx'}) # fails if file exists catch error @configFileHasErrors = true @notifyFailure("Failed to initialize `#{path.basename(@configFilePath)}`", error.stack) From 85ca408b29d904269977e10d832a9d2740d3a284 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 10:20:14 -0400 Subject: [PATCH 15/28] Remove some console logging --- src/config.coffee | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/config.coffee b/src/config.coffee index 0d95ab2ef..e9e4a436e 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -860,7 +860,6 @@ class Config loadUserConfig: -> return if @shouldNotAccessFileSystem() - console.log 'loadUserConfig' try fs.makeTreeSync(path.dirname(@configFilePath)) CSON.writeFileSync(@configFilePath, {}, {flag: 'wx'}) # fails if file exists @@ -891,13 +890,9 @@ class Config return if @shouldNotAccessFileSystem() try - console.trace 'create watch subscription', @watchSubscriptionPromise @watchSubscriptionPromise ?= watchPath @configFilePath, {}, (events) => - console.log events for {action} in events - console.log action, @watchSubscriptionPromise? if action in ['created', 'modified', 'renamed'] and @watchSubscriptionPromise? - console.warn 'request load' @requestLoad() catch error @notifyFailure """ @@ -912,7 +907,6 @@ class Config unobserveUserConfig: -> @watchSubscriptionPromise?.then((watcher) => watcher?.dispose()) @watchSubscriptionPromise = null - console.log 'unobserve' notifyFailure: (errorMessage, detail) -> @notificationManager?.addError(errorMessage, {detail, dismissable: true}) From 8601a5df21237c576749797f55cae96d4e5a3c15 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 10:20:30 -0400 Subject: [PATCH 16/28] Only catch an expected EEXIST error --- src/config.coffee | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/config.coffee b/src/config.coffee index e9e4a436e..2f0952e8a 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -864,9 +864,10 @@ class Config fs.makeTreeSync(path.dirname(@configFilePath)) CSON.writeFileSync(@configFilePath, {}, {flag: 'wx'}) # fails if file exists catch error - @configFileHasErrors = true - @notifyFailure("Failed to initialize `#{path.basename(@configFilePath)}`", error.stack) - return + if error.code isnt 'EEXIST' + @configFileHasErrors = true + @notifyFailure("Failed to initialize `#{path.basename(@configFilePath)}`", error.stack) + return try unless @savePending From 46d5ebb2f4b238b327f564fbc6d02ab95237f86c Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 10:21:02 -0400 Subject: [PATCH 17/28] Only reset settings if the file is empty --- src/config.coffee | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/config.coffee b/src/config.coffee index 2f0952e8a..c7eea02c4 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -872,6 +872,11 @@ class Config try unless @savePending userConfig = CSON.readFileSync(@configFilePath) + userConfig = {} if userConfig is null + + unless isPlainObject(userConfig) + throw new Error("`#{path.basename(@configFilePath)}` must contain valid JSON or CSON") + @resetUserSettings(userConfig) @configFileHasErrors = false catch error @@ -930,11 +935,6 @@ class Config ### resetUserSettings: (newSettings) -> - unless isPlainObject(newSettings) - @settings = {} - @emitChangeEvent() - return - if newSettings.global? newSettings['*'] = newSettings.global delete newSettings.global From 4b7b513b9316aca4a058e5827315a00c1de96a98 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 10:21:47 -0400 Subject: [PATCH 18/28] The method is requestSave() --- src/config.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.coffee b/src/config.coffee index c7eea02c4..85de901db 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -951,7 +951,7 @@ class Config @set(key, value, save: false) for key, value of newSettings if @pendingOperations.length op() for op in @pendingOperations - @debouncedSave() + @requestSave() @pendingOperations = [] return From 18a0a5a8579cfc77a778f077564bb4015cb326e6 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 10:33:46 -0400 Subject: [PATCH 19/28] .set operations should be immediately visible through .get --- spec/config-spec.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/config-spec.coffee b/spec/config-spec.coffee index 14fbd64d0..7beb74558 100644 --- a/spec/config-spec.coffee +++ b/spec/config-spec.coffee @@ -1750,7 +1750,7 @@ describe "Config", -> expect(atom.config.set('foo.bar.str_options', 'One')).toBe false expect(atom.config.get('foo.bar.str_options')).toEqual 'two' - describe "when .set/.unset is called prior to .loadUserConfig", -> + fdescribe "when .set/.unset is called prior to .loadUserConfig", -> beforeEach -> atom.config.settingsLoaded = false From e275a5ff7600f401f8b717952efa604300fa4aee Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 11:21:20 -0400 Subject: [PATCH 20/28] Ensure that .get calls before .requestLoad return .set properties --- spec/config-spec.coffee | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/spec/config-spec.coffee b/spec/config-spec.coffee index 7beb74558..dfc51b29f 100644 --- a/spec/config-spec.coffee +++ b/spec/config-spec.coffee @@ -1753,7 +1753,6 @@ describe "Config", -> fdescribe "when .set/.unset is called prior to .loadUserConfig", -> beforeEach -> atom.config.settingsLoaded = false - fs.writeFileSync atom.config.configFilePath, """ '*': foo: @@ -1762,20 +1761,23 @@ describe "Config", -> ray: 'me' """ - it "ensures that all settings are loaded correctly", -> - atom.config.unset('foo.bar') - expect(atom.config.save).not.toHaveBeenCalled() - atom.config.set('foo.qux', 'boo') - expect(atom.config.save).not.toHaveBeenCalled() - expect(atom.config.get('foo.qux')).toBeUndefined() + it "ensures that early set and unset calls are replayed after the config is loaded from disk", -> + atom.config.unset 'foo.bar' + atom.config.set 'foo.qux', 'boo' + + expect(atom.config.get('foo.bar')).toBeUndefined() + expect(atom.config.get('foo.qux')).toBe 'boo' expect(atom.config.get('do.ray')).toBeUndefined() - atom.config.loadUserConfig() advanceClock 100 + expect(atom.config.save).not.toHaveBeenCalled() + atom.config.loadUserConfig() + + advanceClock 100 waitsFor -> atom.config.save.callCount > 0 runs -> expect(atom.config.get('foo.bar')).toBeUndefined() - expect(atom.config.get('foo.qux')).toBe('boo') - expect(atom.config.get('do.ray')).toBe('me') + expect(atom.config.get('foo.qux')).toBe 'boo' + expect(atom.config.get('do.ray')).toBe 'me' From 28a42034822cdd03f1f87a2718f68d02e7e8ff5f Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 11:22:40 -0400 Subject: [PATCH 21/28] Apply config changes before an initial load, but also replay them --- src/config.coffee | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/src/config.coffee b/src/config.coffee index 85de901db..4fb1c0e20 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -419,17 +419,21 @@ class Config @defaultSettings = {} @settings = {} @scopedSettingsStore = new ScopedPropertyStore + + @savePending = false @configFileHasErrors = false @transactDepth = 0 - @savePending = false - @requestLoad = _.debounce(@loadUserConfig, 100) + + @requestLoad = _.debounce => + @loadUserConfig() + , 100 + + debouncedSave = _.debounce => + @save() + , 100 @requestSave = => @savePending = true - debouncedSave.call(this) - save = => - @savePending = false - @save() - debouncedSave = _.debounce(save, 100) + debouncedSave() shouldNotAccessFileSystem: -> not @enablePersistence @@ -652,7 +656,6 @@ class Config unless @settingsLoaded @pendingOperations.push () => @set.call(this, keyPath, value, options) - return scopeSelector = options?.scopeSelector source = options?.source @@ -674,7 +677,8 @@ class Config else @setRawValue(keyPath, value) - @requestSave() if source is @getUserConfigPath() and shouldSave and not @configFileHasErrors + if source is @getUserConfigPath() and shouldSave and not @configFileHasErrors and @settingsLoaded + @requestSave() true # Essential: Restore the setting at `keyPath` to its default value. @@ -686,7 +690,6 @@ class Config unset: (keyPath, options) -> unless @settingsLoaded @pendingOperations.push () => @unset.call(this, keyPath, options) - return {scopeSelector, source} = options ? {} source ?= @getUserConfigPath() @@ -699,7 +702,7 @@ class Config setValueAtKeyPath(settings, keyPath, undefined) settings = withoutEmptyObjects(settings) @set(null, settings, {scopeSelector, source, priority: @priorityForSource(source)}) if settings? - @requestSave() + @requestSave() if source is @getUserConfigPath and not @configFileHasErrors and @settingsLoaded else @scopedSettingsStore.removePropertiesForSourceAndSelector(source, scopeSelector) @emitChangeEvent() @@ -859,6 +862,7 @@ class Config loadUserConfig: -> return if @shouldNotAccessFileSystem() + return if @savePending try fs.makeTreeSync(path.dirname(@configFilePath)) @@ -870,15 +874,14 @@ class Config return try - unless @savePending - userConfig = CSON.readFileSync(@configFilePath) - userConfig = {} if userConfig is null + userConfig = CSON.readFileSync(@configFilePath) + userConfig = {} if userConfig is null - unless isPlainObject(userConfig) - throw new Error("`#{path.basename(@configFilePath)}` must contain valid JSON or CSON") + unless isPlainObject(userConfig) + throw new Error("`#{path.basename(@configFilePath)}` must contain valid JSON or CSON") - @resetUserSettings(userConfig) - @configFileHasErrors = false + @resetUserSettings(userConfig) + @configFileHasErrors = false catch error @configFileHasErrors = true message = "Failed to load `#{path.basename(@configFilePath)}`" @@ -918,6 +921,7 @@ class Config @notificationManager?.addError(errorMessage, {detail, dismissable: true}) save: -> + @savePending = false return if @shouldNotAccessFileSystem() allSettings = {'*': @settings} From 7737aec3ae00d005788b1693f839d778660e04a1 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 11:25:29 -0400 Subject: [PATCH 22/28] :fire: fdescribe --- spec/config-spec.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/config-spec.coffee b/spec/config-spec.coffee index dfc51b29f..43a730ca8 100644 --- a/spec/config-spec.coffee +++ b/spec/config-spec.coffee @@ -1750,7 +1750,7 @@ describe "Config", -> expect(atom.config.set('foo.bar.str_options', 'One')).toBe false expect(atom.config.get('foo.bar.str_options')).toEqual 'two' - fdescribe "when .set/.unset is called prior to .loadUserConfig", -> + describe "when .set/.unset is called prior to .loadUserConfig", -> beforeEach -> atom.config.settingsLoaded = false fs.writeFileSync atom.config.configFilePath, """ From 19500d1b4087a484387577e046522d8f522236b0 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 11:30:07 -0400 Subject: [PATCH 23/28] Functions are more useful if you call them --- src/config.coffee | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/config.coffee b/src/config.coffee index 4fb1c0e20..2180bcc62 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -702,7 +702,8 @@ class Config setValueAtKeyPath(settings, keyPath, undefined) settings = withoutEmptyObjects(settings) @set(null, settings, {scopeSelector, source, priority: @priorityForSource(source)}) if settings? - @requestSave() if source is @getUserConfigPath and not @configFileHasErrors and @settingsLoaded + if source is @getUserConfigPath() and not @configFileHasErrors and @settingsLoaded + @requestSave() else @scopedSettingsStore.removePropertiesForSourceAndSelector(source, scopeSelector) @emitChangeEvent() From 979f525e3beaeb12ed421d44aa589c6a7e864802 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 11:50:15 -0400 Subject: [PATCH 24/28] Work with spies a little more gracefully --- src/config.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.coffee b/src/config.coffee index 2180bcc62..1ad7fab47 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -429,6 +429,7 @@ class Config , 100 debouncedSave = _.debounce => + @savePending = false @save() , 100 @requestSave = => @@ -922,7 +923,6 @@ class Config @notificationManager?.addError(errorMessage, {detail, dismissable: true}) save: -> - @savePending = false return if @shouldNotAccessFileSystem() allSettings = {'*': @settings} From fa1ebd052992c1c16735db784e246d77c1050198 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 11:52:39 -0400 Subject: [PATCH 25/28] :shirt: lint lint lint --- src/config.coffee | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/config.coffee b/src/config.coffee index 1ad7fab47..ea4551652 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -656,7 +656,7 @@ class Config [keyPath, value, options] = arguments unless @settingsLoaded - @pendingOperations.push () => @set.call(this, keyPath, value, options) + @pendingOperations.push => @set.call(this, keyPath, value, options) scopeSelector = options?.scopeSelector source = options?.source @@ -690,7 +690,7 @@ class Config # * `source` (optional) {String}. See {::set} unset: (keyPath, options) -> unless @settingsLoaded - @pendingOperations.push () => @unset.call(this, keyPath, options) + @pendingOperations.push => @unset.call(this, keyPath, options) {scopeSelector, source} = options ? {} source ?= @getUserConfigPath() @@ -916,7 +916,7 @@ class Config @watchSubscriptionPromise unobserveUserConfig: -> - @watchSubscriptionPromise?.then((watcher) => watcher?.dispose()) + @watchSubscriptionPromise?.then (watcher) -> watcher?.dispose() @watchSubscriptionPromise = null notifyFailure: (errorMessage, detail) -> From 0c021f777e0fe2e6d5dc3b0cb1e1528e7f0caf13 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 14:38:49 -0400 Subject: [PATCH 26/28] Initialize the new flag and pendingOperations in clear() --- src/config.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/config.coffee b/src/config.coffee index ea4551652..56b2808ca 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -399,8 +399,6 @@ class Config # Created during initialization, available as `atom.config` constructor: ({@notificationManager, @enablePersistence}={}) -> - @settingsLoaded = false - @pendingOperations = [] @clear() initialize: ({@configDirPath, @resourcePath, projectHomeSchema}) -> @@ -420,9 +418,11 @@ class Config @settings = {} @scopedSettingsStore = new ScopedPropertyStore + @settingsLoaded = false @savePending = false @configFileHasErrors = false @transactDepth = 0 + @pendingOperations = [] @requestLoad = _.debounce => @loadUserConfig() From ff8e21a20e9f38563df07196d3f1d270855c63a0 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 14:39:08 -0400 Subject: [PATCH 27/28] Let the debouncer debounce --- src/config.coffee | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/config.coffee b/src/config.coffee index 56b2808ca..a585a433a 100644 --- a/src/config.coffee +++ b/src/config.coffee @@ -956,9 +956,7 @@ class Config @set(key, value, save: false) for key, value of newSettings if @pendingOperations.length op() for op in @pendingOperations - @requestSave() @pendingOperations = [] - return getRawValue: (keyPath, options) -> unless options?.excludeSources?.indexOf(@getUserConfigPath()) >= 0 From fb4d7ee5e5c62de5492df66b756515b776f94f8e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 12 Sep 2017 14:39:17 -0400 Subject: [PATCH 28/28] Clean the pending operations too --- spec/config-spec.coffee | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/config-spec.coffee b/spec/config-spec.coffee index 43a730ca8..bcf50c268 100644 --- a/spec/config-spec.coffee +++ b/spec/config-spec.coffee @@ -14,6 +14,7 @@ describe "Config", -> atom.config.configDirPath = dotAtomPath atom.config.enablePersistence = true atom.config.settingsLoaded = true + atom.config.pendingOperations = [] atom.config.configFilePath = path.join(atom.config.configDirPath, "atom.config.cson") afterEach ->