Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repl: fix global object in repl #28049

Closed
wants to merge 1 commit into from

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Jun 4, 2019

The global object in repl conetxt is copied from main context, which breaks the behavior of instanceof.

This change reverts #25731. cc @BridgeAR

Fixes: #27859
Refs: #25731

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

The global object in repl conetxt is copied from main context,
which breaks the behavior of `instanceof`.

This change reverts nodejs#25731.

Fixes: nodejs#27859
Refs: nodejs#25731
@starkwang starkwang requested a review from BridgeAR June 4, 2019 09:46
@BridgeAR
Copy link
Member

BridgeAR commented Jun 4, 2019

@starkwang the problem is something fundamental with our current REPL design. There is no perfect fix for the mentioned issue and reverting this commit would only fix a part of the reported issue. It also does not seem to be a good idea to do a complete revert of that commit, since it improved some autocompletion things.

The former global list worked as exclusion list. The list would easily become outdated and what we really want is to exclude all original primordial keys, even when the language adds new built-ins. We extend our global object with e.g., process and we should find the "diff" to extend the context with these added entries.

That is possible with the following patch:

diff --git a/lib/repl.js b/lib/repl.js
index 12461d67de..6293363640 100644
--- a/lib/repl.js
+++ b/lib/repl.js
@@ -875,8 +875,11 @@ REPLServer.prototype.createContext = function() {
       context = vm.createContext();
     });
     for (const name of Object.getOwnPropertyNames(global)) {
-      Object.defineProperty(context, name,
-                            Object.getOwnPropertyDescriptor(global, name));
+      // Only set properties on the context that do not exist as primordial.
+      if (!(name in primordials)) {
+        Object.defineProperty(context, name,
+                              Object.getOwnPropertyDescriptor(global, name));
+      }
     }
     context.global = context;
     const _console = new Console(this.outputStream);
diff --git a/test/parallel/test-repl-context.js b/test/parallel/test-repl-context.js
index 287d8adc29..88bd47a928 100644
--- a/test/parallel/test-repl-context.js
+++ b/test/parallel/test-repl-context.js
@@ -16,11 +16,21 @@ const stream = new ArrayStream();
     useGlobal: false
   });
 
+  let output = '';
+  stream.write = function(d) {
+    output += d;
+  };
+
   // Ensure that the repl context gets its own "console" instance.
   assert(r.context.console);
 
   // Ensure that the repl console instance is not the global one.
   assert.notStrictEqual(r.context.console, console);
+  assert.notStrictEqual(r.context.Object, Object);
+
+  stream.run(['({} instanceof Object)']);
+
+  assert.strictEqual(output, 'true\n> ');
 
   const context = r.createContext();
   // Ensure that the repl context gets its own "console" instance.

We extend the global primordial object with extra functions like uncurryThis and ObjectPrototype but that does not seem to be an actual issue because we do not use those properties in our global object.

@starkwang starkwang closed this Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REPL: {} instanceof Object === false with nodejs 11 and nodejs 12
2 participants