[core] Handle invalid config on load #750
Conversation
|
Thanks a ton for the review @mauricioszabo! |
| if (configFilePath) { | ||
| const configFileData = CSON.readFileSync(configFilePath); | ||
| config.resetUserSettings(configFileData); | ||
| CSON.readFile(configFilePath, (error, data) => { | ||
| // Duplicated from `./src/config-file.js`.reload() | ||
| if (error) { | ||
| console.log(error.message); | ||
| } else { | ||
| config.resetUserSettings(data); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| return config; |
There was a problem hiding this comment.
Sorry to look at this so late, but...
config is returned later. Is it okay to do this stuff async?
I suppose without much deep knowledge about what this code does, the important question is "does config.resetUserSettings(data) modify the value of config, or is it done only for side-effects?
Is moving this to async possibly returning a different value of config than before, due to race conditions with async stuff "maybe finishing, maybe not finishing (probably not finishing)" before the return config; is done?
[ EDIT TO ADD: Or if config.resetUserSettings(data) modifies config, does it do so in a way that updates config for the caller of getConfig()? Is config passed to the caller of getConfig() by reference, or by copy? I dunno without looking into it more, personally. ]
When Pulsar loads a user's config while the application is open there is proper error handling to warn of an invalid config. But it seems that if Pulsar is first starting up and finds an invalid config, it'll crash silently, while reporting that it is still running.
While I would've much preferred to not have to duplicate this error checking at all, unfortunately the other error checking is mostly special by being attached to an emitter, which during initial startup we don't have that same feature. So it seemed simple enough otherwise to implement.
You can view the live changes error handling below:
pulsar/src/config-file.js
Lines 112 to 117 in 5f3e40a
But as for here, we will add just a small bit of error handling, which if it fails, will load the default config, just like the config file doesn't exist, as well as log a message to the terminal. We can from here let the live reloading take care of alerting the user.
Resolves #409