Ability to launch SourceJS via NodeJS Script #241
Conversation
…sing global.pathToApp
…er script file using require()
| silent = _silent; | ||
|
|
||
| var pathToApp = basePath || path.dirname(require.main.filename); | ||
| var pathToApp = basePath || global.pathToApp; |
There was a problem hiding this comment.
Using globals is considered a bad practice, since it requires some special environment setup of the callee. I'll need to double-check how these changes affect the current setup.
From other side require.main.filename is also limited, and was the reason to be replaced, since you execute SourceJS code from other location.
There was a problem hiding this comment.
Globals are indeed a bad practice.
However, beyond my reasoning for running from other location, it was also inconsistent with the rest of the SourceJS code base.
I used the global.pathToApp, since it was used by the rest of the core files (see processMd, specUtils, etc). =)
There was a problem hiding this comment.
You are right, it's just the rest of the codebase also needs to be improved in that sense :) But as I said, there's no better alternative, except passing the properties directly, as it's also done here in the main execution scenario.
But first let's discuss other SourceJS calling scenarios, since we may find a better option (following this discussion #240 (comment)).
app.js
Outdated
|
|
||
| spawn('./node_modules/grunt-cli/bin/grunt', [commander.postGrunt, '--port='+port], {stdio: 'inherit'}) | ||
| module.exports = function () { | ||
| var serverOpts = global.opts.core.server; |
There was a problem hiding this comment.
please keep the original 4 space indentation
app.js
Outdated
| var spawn = require('cross-spawn'); | ||
|
|
||
| spawn('./node_modules/grunt-cli/bin/grunt', [commander.postGrunt, '--port='+port], {stdio: 'inherit'}) | ||
| module.exports = function () { |
There was a problem hiding this comment.
please create a named function and add it into exports below the file
There was a problem hiding this comment.
Something like
module.exports = {
startServer: function() { ... }
};
Work for you?
There was a problem hiding this comment.
It would be cleaner like this:
var startServer = function() {}
module.exports = {
startServer: startServer
}
It's a nice practice to keep module exports below, so everybody could easily overview the exported interface.
|
I made the changes so that module.exports and fixed the indent spacing to 4 spaces. |
|
I think adding the ability to pass an "options" object to the startServer function might need to wait for the next release. Just looking it over, seems like it'll need more thinking since host/port are stored in global options as well, and used in other files. I'm not sure of the time-of-use (just briefly scanning) if there would have already been used, and thus too late to modify in startServer |
|
Thanks again for the PR, now everything looks fine. Let's continue with next stage of passing options as soon as you're ready. Cheers |
2 participants
These are the fixes for #240