[Fix] Panic when using npm start on OSX fresh dev setup. Resolves #1017. #1016
Conversation
Panic occurs when the path ~/Library/Preferences/Soundnode does not exist (ie a fresh dev setup) since `getUserConfig()` uses `fs.statSync(..)`, which throws an exception instead of returning an error if the directory does not exist. If the directory mention above does exist, all is fine. When it doesn't, such as on a fresh dev setup, every time you run 'npm start' you get a panic and the app crashes. My solution to this is to wrap `fs.statSync(..)` in a try catch and problem solved. Furthermore I added in a guard which checks the type of `userConfigPath`. The reason for this guard is so we can throw a useful exception rather than a generic undefined or null exception.
JackMordaunt
changed the title
| // any directory along the path doesn't exist. | ||
| // Solution: use try catch. | ||
| try { | ||
| var fi = fs.statSync(userConfigPath); |
There was a problem hiding this comment.
can you rename the variable to something more clear please?
There was a problem hiding this comment.
Given the tiny scope of the variable I felt the name to be adequate.
What would you consider to be a more clear name? Maybe something like configFileInfo?
There was a problem hiding this comment.
configFileInfo would be more explicit,
Since the variable is never mutated, could you change it to a constant?
| @@ -38,12 +38,26 @@ const configuration = { | |||
| userConfigPath = `${userHome}/Library/Preferences/Soundnode`; | |||
| } | |||
|
|
|||
| // Guard to assert type of string. | |||
| if (typeof userConfigPath !== "string") { | |||
There was a problem hiding this comment.
can you tell me if there is a specific case for this check to be necessary? thanks
There was a problem hiding this comment.
Since userConfigPath is initially set to null, if the platform is not linux, windows, or darwin, then it never gets assigned to a string. Thus if userConfigPath is still a null value the platform is obviously not supported, so an exception is thrown to let the user know why the userConfigPath value could not be set properly.
The bug was actually quite hard to find because all I had to go on was a generic null reference exception, this guard makes the exception explicitly clear.
| // fs.statSync will throw an exception if | ||
| // any directory along the path doesn't exist. | ||
| // Solution: use try catch. | ||
| try { |
There was a problem hiding this comment.
what are other solutions instead try catch?
There was a problem hiding this comment.
@weblancaster It appears stat throws an error, so we will need to catch the error or rethink the entire functionality of the configLocation
EDIT: nevermind, the callback returns an error variable.
It might be better to be safe so the startup of the application doesn't fail though.
|
@JackMordaunt thanks for contributing and sorry for my delay in reply to you. can you take a look the comments I left please? thanks. |
|
this looks good to me, any concerns about merging @weblancaster ? |
|
@weblancaster Going to merge this so we can get a new release out. |
|
@jakejarrett the code looks good but have you test before merging? if so and worked as expected no worries. |
|
Yeah tested on Windows, Linux & Mac OSX. |
Panic occurs when the path ~/Library/Preferences/Soundnode does not exist (ie a fresh dev setup) since
getUserConfig()usesfs.statSync(..), which throws an exception instead of returning an error if the directory does not exist.If the directory mentioned above does exist, all is fine. When it doesn't, such as on a fresh dev setup, every time you run 'npm start' you get a panic and the app crashes.
My solution to this is to wrap
fs.statSync(..)in a try catch and problem solved.Furthermore I added in a guard which checks the type of
userConfigPath. The reason for this guard is so we can throw a useful exception rather than a generic undefined or null exception.