App: create default config file in launch script #3630
Conversation
app.js
Outdated
| @@ -49,7 +49,8 @@ const path = require('path'); | |||
| try { | |||
| require.resolve('sockjs'); | |||
| } catch (e) { | |||
| throw new Error("Dependencies unmet; run npm install"); | |||
| console.error('Dependencies are unmet; run npm install before launching Pokemon Showdown again.'); | |||
| process.exit(1); | |||
There was a problem hiding this comment.
This isn't dependent-friendly. If another project tries to load pokemon-showdown/app but it fails, it should result in an exception, not in the whole application exiting.
There was a problem hiding this comment.
I'm curious what the improvement is.
There was a problem hiding this comment.
With a throw, you can wrap the module load with try-catch.
process.exit() goes through the native layer and can't be prevented.
There was a problem hiding this comment.
Yes, I'm wondering why anyone would want to use process.exit() instead.
There was a problem hiding this comment.
@xfix would be able to explain better, since the problems with simply throwing an error instead of exiting don't happen on my system. Maybe it's dependent on OS or the version of Node running
app.js
Outdated
| @@ -62,14 +63,16 @@ try { | |||
| if (err.code !== 'MODULE_NOT_FOUND') throw err; // should never happen | |||
|
|
|||
| // Copy it over synchronously from config-example.js since it's needed before we can start the server | |||
| console.log("config.js doesn't exist - creating one with default settings..."); | |||
| console.error("config.js doesn't exist. Creating one with default settings..."); | |||
There was a problem hiding this comment.
I'm also fine with just not creating one at all, if it's done in launcher.
|
Changed this back to throwing errors instead of making the process exit, moved creating the default config file to the launch script |
Morfent
changed the title
|
Still waiting for @xfix |
app.js
Outdated
| // Check for dependencies | ||
| try { | ||
| require.resolve('sockjs'); | ||
| } catch (e) { | ||
| throw new Error("Dependencies unmet; run npm install"); | ||
| throw new Error('Dependencies are unmet; run node pokemon-showdown or npm install before launching Pokemon Showdown again.'); |
There was a problem hiding this comment.
Just recommend node pokemon-showdown, it's needed to set up config.js anyway.
pokemon-showdown
Outdated
|
|
||
| // Make sure we're Node 6+ | ||
|
|
||
| try { | ||
| eval('{ let [a] = [1]; }'); | ||
| } catch (e) { | ||
| console.log("We require Node.js v6 or later; you're using " + process.version); | ||
| process.exit(); | ||
| process.exit(0); |
There was a problem hiding this comment.
Should possibly be an error code?
There was a problem hiding this comment.
Would using 1 (SIGHUP) be appropriate?
There was a problem hiding this comment.
Apparently the most appropriate exit code is EX_UNAVAILABLE (69)
There was a problem hiding this comment.
There was a problem hiding this comment.
So following Node's conventions for error codes, it should be 128 + 69
There was a problem hiding this comment.
What does using the wrong Node version have to do with receiving a signal?
There was a problem hiding this comment.
...didn't notice that. I'll take a look at how Node deals with exit codes internally, since I'm not positive exiting with errno EX_UNAVAILABLE will do what we're intending to do, judging from the other exit codes below 128
There was a problem hiding this comment.
It looks like it uses C error codes judging from the values for process.exitCode used in its unit tests
|
Changed the errno to |
|
Why 128? |
|
Maybe just use code |
|
Changed the errno to |
app.js
Outdated
| if (Config.watchconfig) { | ||
| let configPath = require.resolve('./config/config'); | ||
| fs.watchFile(configPath, (curr, prev) => { | ||
| require('fs').watchFile(configPath, (curr, prev) => { |
There was a problem hiding this comment.
Because this is the only place fs is required at all in app.js anymore
There was a problem hiding this comment.
Convention is to have requires at the top. Making an exception for "only one require" just makes the code inconsistent, and you're going to have to change this if you add more uses anyway.
There was a problem hiding this comment.
But the main thing is consistency. It's not consistent to have require('fs').watchFile in some parts of the codebase and fs.watchFile in other parts of the codebase.
There was a problem hiding this comment.
Who told you that? I'm personally mostly against inline requires.
There was a problem hiding this comment.
Also I did say literally on the same comment thread that I disagreed with kota.
There was a problem hiding this comment.
Let me put it this way, I'm fine with using require() inline if it's the only place the module is required in the entire codebase, and it's an optional module. Otherwise, put it at the top.
|
Added back the |
Brought up in #3536 (comment). I don't think it'd be feasible to install the dependencies and import newly recreated config files from app.js because of how much of a hassle that caused with the launch script trying to accomplish the same thing.