Use vm.runInContext

[n-riesco]

Moved discussion from n-riesco/ijavascript#117 (comment) :

---

I would solve this issue like this:

1. replace the following line of code:

ContextifyScript.Script.runInThisContext();

with

ContextifyScript.Script.runInContext(currentContext);

2. Check for first error

If the code errors out, parse the error to check for this pattern:

SyntaxError: Identifier '*' has already been declared

if this pattern matches, create a new context to be used across the current session, and run the code again, any error created afterwards is propagated to the user.

function createContext() {
  const sandbox = {
    Buffer,
    clearImmediate,
    clearInterval,
    clearTimeout,
    setImmediate,
    setInterval,
    setTimeout,
    console,
    process,
    // ...other ijavascript-y needed context
  };
  sandbox.global = sandbox;
  return sandbox;
}

try {
	ContextifyScript.Script.runInContext(currentContext);
} catch(e) {
   if(DOUBLE_DECL_PATTERN.test(e.message)) {
      currentContext = vm.createContext(createContext());
      return ContextifyScript.Script.runInContext(currentContext);
   }
   throw e
}

I have not looked into the ijavascript code yet, but I am hoping to get some feedback on this before I do so.

[n-riesco]

Moved from n-riesco/ijavascript#117 (comment) :

---

@Bamieh At the very beginning of IJavascript, I experimented with runInContext, but I ruled it out because this approach is riddled with issues, e.g.:

[] instanceof Array // true

vm.runInContext("[] instanceof Array", vm.createContext({Array})); // false

---

Another difficulty with this idea of recreating the context is that the state of previous context would be lost.

---

The way I see this issue is that IJavascript is an enriched REPL for Node.js and it aims at behaving the same way Node.js's REPL does.

When I use IJavascript as a REPL, I prefer to use var to define variables in the global context, and let and const inside functions.

When I use IJavascript to run a a script, then the normal rules for const and let apply, and this isn't an issue.

---

Picking up @rgbkrk 's suggestion and your idea, I could implement $$.restart() to trigger a kernel restart (so that the user wouldn't have to do the same by other means).

[n-riesco]

Moved from n-riesco/ijavascript#117 (comment) :

---

@n-riesco why would you pass Array to the new context 🤔 , it gives false because the Array you passed is pointing to a different location in memory than the one created by [] since both run in a different v8 contexts. By not passing Array, this will evaluate to true as it should. The createContext I provided is valid for production uses.

To solve the previous context issue, we just track added objects on the global (observers or manually) and propagate them in the new context.

---

Implementing a restart is not a good idea imo, since it might break other code blocks depending on previous code. If the code is to be run in an isolated scope, anyone can write this:

{
const ..
}

#blocks_are_the_new_iffe 😆

[n-riesco]

@Bamieh

why would you pass Array to the new context thinking ,

I was doing something like this:

vm.runInContext("[] instanceof Array", vm.createContext(global));

it gives false because the Array you passed is pointing to a different location in memory than the one created by [] since both run in a different v8 contexts. By not passing Array, this will evaluate to true as it should.

What would be the criteria to decide what was into the context?

The createContext I provided is valid for production uses.

🤔

To solve the previous context issue, we just track added objects on the global (observers or manually) and propagate them in the new context.

Observers are gone. Perhaps a Proxy would do the trick.

Potentially, this may break for very large-memory objects (but let's ignore this for the time being).

Preserving the global object only solves part of the problem:

• what about the event loop and callbacks waiting to be run or triggered?
• what about the context of those callbacks?

Implementing a restart is not a good idea imo, since it might break other code blocks depending on previous code.

🤔 (in the cases that it breaks, users should re-run the necessary code; $$.restart() is just a suggestion for convenience).

If the code is to be run in an isolated scope, [...]

yes, this is useful, when the user doesn't need to carry definitions across notebooks cells.

[rgbkrk]

instanceof only checks if Array.prototype is on an object's [[Prototype]] chain. You generally want to be using Array.isArray. However, that's not the issue you're running into here -- Array is a built in within v8. When you create a [], it's done directly by v8.

kylek@nfml-83G ~$ v8
V8 version 5.1.281.47 [sample shell]
> Array
function Array() { [native code] }
> Array.isArray([])
true
> [] instanceof Array
true

Effectively, you shouldn't pass Array as part of creating the vm context.

The way I see this issue is that IJavascript is an enriched REPL for Node.js and it aims at behaving the same way Node.js's REPL does.

Thank you! This is exactly what I want out of the ijavascript kernel.

When I use IJavascript as a REPL, I prefer to use var to define variables in the global context, and let and const inside functions.

Me too, that's what I've been doing as well.

[Bamieh]

What would be the criteria to decide what was into the context?

The window/global object is a circular reference to the context object, but it is not the object itself.

What needs to be inside the context is what node puts on the context, just logging the global object will show you what is needed:

Object.keys(global);
[ 'console',
  'global',
  'process',
  'Buffer',
  'clearImmediate',
  'clearInterval',
  'clearTimeout',
  'setImmediate',
  'setInterval',
  'setTimeout',
  'module',
  'require' ]

Observers are gone. Perhaps a Proxy would do the trick.

yea, i meant observers as a concept regardless of the implementation, using proxies, a library, or manual looping.

Potentially, this may break for very large-memory objects (but let's ignore this for the time being).

The passed context objects are passed by reference, so you are just passing reference points of those objects already in memory.

The way I see this issue is that IJavascript is an enriched REPL for Node.js and it aims at behaving the same way Node.js's REPL does.

It will still behave like a REPL regardless if we are running each individual snippet in a new vm or the same one (since we are sharing the context between them). For the end user,

The current behavior of const and let is unexpected, What you do prefer is valid, but it is forcing users to behave in a certain way, which is not the preferred route if we can do something about it, IMO.

[n-riesco]

@Bamieh

Without a solution for preserving the event loop, callbacks and the corresponding contexts, wouldn't this approach still require that users re-run previous code?

---

What needs to be inside the context is what node puts on the context, just logging the global object will show you what is needed: Object.keys(global)

We'd also need to preserve all the modules available in a REPL session? Is there a way to determine this programmatically?

The passed context objects are passed by reference, so you are just passing reference points of those objects already in memory.

This wouldn't work. We'd need to clone the context before running the execution request, because the execution request may modify the context before encountering a const; e.g. : a += 1; const b=0;.

It will still behave like a REPL regardless if we are running each individual snippet in a new vm or the same one (since we are sharing the context between them).

My feeling is that this is a project in itself that would require constant maintenance to keep up with changes in Node's REPL.

The current behavior of const and let is unexpected, What you do prefer is valid, but it is forcing users to behave in a certain way, which is not the preferred route if we can do something about it, IMO.

If, as you think, this is an unexpected behaviour for users, the place to fix it is https://github.com/nodejs/node .

Personally, I think the const behaviour requested in n-riesco/ijavascript#117 is ill-defined. Only the user can determine whether re-running a const is intended or a mistake.

[Bamieh]

We'd also need to preserve all the modules available in a REPL session? Is there a way to determine this programmatically?

passing the module and require in the context does the job.

If, as you think, this is an unexpected behaviour for users, the place to fix it is nodejs/node .

Although this is problematic for any REPL, node people agreed to not fix this issue.

Personally, I think the const behaviour requested in n-riesco/ijavascript#117 is ill-defined. Only the user can determine whether re-running a const is intended or a mistake.

we can leave the behavior as is for now then

[n-riesco]

@Bamieh

passing the module and require in the context does the job.

I'm not sure I understand, I've tried this but it fails:

> vm.runInContext("child_process", vm.createContext({require, module}));
evalmachine.<anonymous>:1
child_process
^

ReferenceError: child_process is not defined
    at evalmachine.<anonymous>:1:1
    at ContextifyScript.Script.runInContext (vm.js:35:29)
    at Object.runInContext (vm.js:89:6)
    at repl:1:4
    at sigintHandlersWrap (vm.js:22:35)
    at sigintHandlersWrap (vm.js:73:12)
    at ContextifyScript.Script.runInThisContext (vm.js:21:12)
    at REPLServer.defaultEval (repl.js:340:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)

Although this is problematic for any REPL, node people agreed to not fix this issue.

I remember reading about it, but now I couldn't find a link. Do you have a link to that decision?

we can leave the behavior as is for now then

OK, but if you make any progress on this subject, please, don't hesitate to report back here.

[rgbkrk]

That would be:

vm.runInContext("require('child_process'); child_process", vm.createContext({require, module}));

[n-riesco]

@rgbkrk There's no need for require('child_process) inside a REPL session:

$ node
> child_process
{ ChildProcess: 
   { [Function: ChildProcess]
     super_: 
      { [Function: EventEmitter]
        EventEmitter: [Circular],
        usingDomains: true,
        defaultMaxListeners: [Getter/Setter],
        init: [Function],
        listenerCount: [Function] } },
  fork: [Function],
  _forkChild: [Function],
  exec: [Function],
  execFile: [Function],
  spawn: [Function],
  spawnSync: [Function: spawnSync],
  execFileSync: [Function: execFileSync],
  execSync: [Function: execSync] }
> 

Anyway, I don't think the lack of pre-required modules in a REPL session would be a show-stopper (it's just more convenient).

[Bamieh]

I'll try to hack a little with it on the weekend see if i can come up with something simple.

Here is a link to the node discussion nodejs/node#8441

[rgbkrk]

That seems like a need for passing child_process into the context:

> var context = vm.createContext({require, child_process})
undefined

> vm.runInContext('child_process', context)
{ ChildProcess:
   { [Function: ChildProcess]
     super_:
      { [Function: EventEmitter]
        EventEmitter: [Circular],
        usingDomains: true,
        defaultMaxListeners: [Getter/Setter],
        init: [Function],
        listenerCount: [Function] } },
  fork: [Function],
  _forkChild: [Function],
  exec: [Function],
  execFile: [Function],
  spawn: [Function],
  spawnSync: [Function: spawnSync],
  execFileSync: [Function: execFileSync],
  execSync: [Function: execSync] }

[rgbkrk]

OMG I finally read the rest of the above more thoroughly. I didn't realize that built-in modules got "auto-required" in some sense. This whole time I've been working at the node repl I've been require'ing built in modules.

[rgbkrk]

Since Object.keys doesn't list these modules I looked at Object.getOwnPropertyNames(global). That does list all the modules that are already loaded (in addition to all the actual JS / v8 builtins).

[n-riesco]

@Bamieh As an initial test, I'd use a notebook like this:

// In[1]:
let x = 0;
const $x = $$.display("x");
setInterval(() => $x.text(`x: ${x}`), 1000);

console.log("The display below should show 'x: 0' until next cell is run");


// In[2]:
let x = 2

console.log("Now, the display in the cell above should show 'x: 2'");

[ken-okabe]

For

SyntaxError: Identifier '*' has already been declared

issue, I think this is due to using ES6 const instead of the lagacy var, at least for my situation.

So, what I understand is for a given code :

const a = 1; 

Re-running the cells will override

const a = 1; 

on the same context, for some reason(which I don't know why the implementation is like this, but probably, for big data operation Python code don't want to lose the previous result.)

In fact, this is very easy to solve when the kernel pass the entire JavaScript source-code to the evaluation process, to add ( to the head and ) to tail makes

(
const a = 1;
)

then, this constant value or any other internal constant values are safely to be isolated from the override of re-running evaluation.

[ken-okabe]

I must comment this issue which a code with const declaration generates

SyntaxError: Identifier '*' has already been declared

on / from the second time evaluations should be flagged as Bug. This should not be happened.
Looking at the source code, the developer sticks to var and does not take advantage of const, so I suppose the developer does not notice how serious this problem is, and this issue lose the reputation of the library.

It is obvious, the JypyterNotebook document is static including the JavaScript source-code, and when the source is static and identical, evaluation result should be also identical. Running process for the evaluation for the second time differs to the first time is a phenomenon that should never happen.

[n-riesco]

@kenokabe

In fact, this is very easy to solve when the kernel pass the entire JavaScript source-code to the evaluation process, to add ( to the head and ) to tail makes

Assuming you meant the use of brackets { ... } to define a block scope as defined in the ES6 standard: this isn't a solution, because Jupyter notebooks are run one cell after another. If a cell was enclosed in brackets, definitions in that cell wouldn't be visible from other cells.

I must comment this issue which a code with const declaration generates
SyntaxError: Identifier '*' has already been declared
on / from the second time evaluations should be flagged as Bug. This should not be happened.

Why shouldn't it happen? Let's imagine this example. Someone writes a notebook and by mistake they type the same cell twice. If this cell defines a variable using const, how can IJavascript tell whether this is an actual error or the user indeed wanted to run a cell twice?

Looking at the source code, the developer sticks to var and does not take advantage of const, so I suppose the developer does not notice how serious this problem is, and this issue lose the reputation of the library.

I do this is because IJavascript was originally written in ES5 and it's backwards-compatible down to node v0.10.

Also, please, note that the project uses the linter ESLint to prevent the shadowing of identifiers.

It is obvious, the JypyterNotebook document is static including the JavaScript source-code, and when the source is static and identical, evaluation result should be also identical. Running process for the evaluation for the second time differs to the first time is a phenomenon that should never happen.

This statement isn't correct. hydrogen, nteract, the Jupyter notebook and JupyterLab treat notebooks as REPL sessions. As far as I'm aware, only nbconvert and nbviewer treat a Jupyter notebook as an static document.