Replay "allow optional REPL" on new dev #69
Conversation
src/node.cc
Outdated
| // add allow_repl parameter to JS process so we can use it in | ||
| // bootstrap_node.js | ||
| process_object->Set(env->context(), | ||
| String::NewFromUtf8(env->isolate(), "allow_repl"), |
There was a problem hiding this comment.
What do you think of changing allow_repl to _allowRepl in the JS code to be consistent with _forceRepl?
src/node.cc
Outdated
| @@ -3341,6 +3341,13 @@ void LoadEnvironment(Environment* env) { | |||
| // who do not like how bootstrap_node.js sets up the module system but do | |||
| // like Node's I/O bindings may want to replace 'f' with their own function. | |||
| Local<Value> arg = env->process_object(); | |||
| Local<Object> process_object = env->process_object(); | |||
There was a problem hiding this comment.
Would it be an option to change arg to type Local<Object> or use dynamic _cast<Local<Object>>(arg) instead of the new process_object variable?
There was a problem hiding this comment.
I looked at it when I wrote the changes, but couldn't find a way to convert the handle from Value to Object. Turns out, casting the objects IN the handles is actually easy. yay!
src/node_lib.h
Outdated
| @@ -80,9 +80,12 @@ bool eventLoopIsRunning(); | |||
| * not called. | |||
| * @param program_name The name for the Node.js application. | |||
| * @param node_args List of arguments for the Node.js engine. | |||
| * @param allow_repl Controls wether the node.js REPL gets spawned when no | |||
| * script is given on initialization. | |||
There was a problem hiding this comment.
Is this correct? Isn't the REPL also spawned when there is a script given but stdin is an interactive terminal?
There was a problem hiding this comment.
The way I understood, stdin only is an interactive terminal when no file is given, but also i may just also be completely misunderstanding the way this works... I mean, with the usual nodejs usage, where you give a script as a CLI argument, stdin still is the terminal, right? But the REPL is not spawned, which is why I gave that description in the doc. Feel free to correct me though...
lib/internal/bootstrap_node.js
Outdated
|
|
||
| } else if (!process.allow_repl && NativeModule.require('tty').isatty(0)) { | ||
| console.log('Embedded mode'); | ||
| evalScript('[eval]'); |
There was a problem hiding this comment.
Can we add the if-check from line 216 here? (if (process._eval != null) { })
There was a problem hiding this comment.
I think @cmfcmf said that we need to do the evalScript call, otherwise the code won't run at all. Let's wait for his input?
src/node.cc
Outdated
|
|
||
| // add allow_repl parameter to JS process so we can use it in | ||
| // bootstrap_node.js | ||
| process_object->Set(env->context(), |
There was a problem hiding this comment.
This introduces a new warning when compiling with clang:
../src/node.cc:3348:3: warning: ignoring return value of function declared with 'warn_unused_result' attribute
[-Wunused-result]
fixes issues pointed out in PR review
Replay "allow optional REPL" on new dev
History can be found in PR #66.
gives us full control over REPL spawn when running in embedded mode
should provide full backwards compatibility.
old Start() behaviour is maintained, as REPL will only spawn when no js-file is provided upon call of Start()
fixes #34