repl: properly handle repl.repl

[BridgeAR]

The repl property is set so that it's possible to inspect the
instances own properties during runtime. This was never tested and
it was also only exported in case the instance was started with
.start() instead of using the constructor directly. In case that
more than a single instance was created, all instances got access
to the first instance.

From now on the repl property is only exported in case the repl is
starte as standalone program.

Refs: #30928 (comment)

Checklist

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

[addaleax]

From now on the repl property is only exported in case the repl is
starte as standalone program.

Why is this not useful for other REPLs?

[BridgeAR]

@addaleax I tried to think of ways to expose the correct instance in case multiple instances are used but I could not come up with a proper solution.
If you can think of a good implementation, please let me know!
The only other easy implementation would be to expose this property only as long as a single REPL instance is started and as soon as a second one is started, that it's removed completely. That would be surprising behavior though in case someone from the first one did access the property before and can't afterwards (unlikely but possible).

[Trott]

The doc checkbox is checked, but there are no doc changes. Seems like this would warrant a documentation change?

[BridgeAR]

@Trott I am hesitant to document this feature. It should only be useful for people who actually work on the REPL itself (as in change the REPLs code). I added a code comment to indicate that.

[BridgeAR]

@nodejs/tsc this could use another review :)

[Trott]

LGTM if CITGM and CI are green. (CITGM seems unlikely to uncover something here, but I'd prefer to be especially cautious anyway.)

[Trott]

@addaleax Is Ruben's explanation above persuasive for you? Or would you prefer we not make this change?

[addaleax]

@Trott I mean, not really, but it doesn’t make a huge difference anyway, I think? It’s not like many people use the REPL module programmatically. But for those, this seems like this change is making things a tiny bit more difficult without a real need to do so.

[BridgeAR]

@addaleax

this seems like this change is making things a tiny bit more difficult without a real need to do so.

I strongly disagree on that. The current behavior seems very wrong. It is possible to change from one REPL instance the internal state of another REPL instance. That should never be possible.
This property should only be useful for users who actually work on the REPL code and who ever does that is still able to access the property in case we are able to verify that the accessing REPL instance is indeed the correct one.

[BridgeAR]

@addaleax please use the request changes in case you are against landing this as is. I will wait a couple more days and land it afterwards otherwise.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/28101/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2178/

[BridgeAR]

As a note: this does actually improve the experience for me as someone who works on the REPL by exposing the correct instance during testing. That did not work properly before.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/28116/

[addaleax]

@BridgeAR As you can see, I’m not blocking this.

It is possible to change from one REPL instance the internal state of another REPL instance. That should never be possible.

That just doesn’t seem to be true.

[BridgeAR]

@addaleax

That just doesn’t seem to be true.

Please elaborate. The following is for example possible:

1. Create two dummy files replIn and replOut.
2. Start:

const fs = require('fs');
const repl = require('repl');

// First REPL instance
repl.start({
  input: fs.ReadStream('./replIn'),
  output: fs.WriteStream('./replOut')
});

// This should not access the state of `firstRepl`, but it does!
repl.start();

3. Then execute e.g.,: repl.repl.output.write('foobar')

There's no output in the current (second) instance. Instead, it's written directly to the first REPL instances output stream (replOut).

[addaleax]

@BridgeAR I don’t consider repl.repl to be internal state of any REPL instance. It’s simply (until now) an utility that helps access the first-started REPL instance. I don’t see how setting it in fewer cases than we do now, which is what this PR does, improves something.

However, this is really not worth arguing about imo – I’m unsubscribing from this thread, and you can feel free to go ahead with what you think is right.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/28145/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/28161/

[BridgeAR]

Landed in 84c426c 🎉