From issue #38139

@aduh95 I'll try to do it thanks

Hi! Thanks for sending the PR. Would it be possible to add tests to make sure this is fixing the issue?

How would you suggest to test this? run the same commands and check no error is printed to screen OR do more of unit test to the specific preview function?

You can add a test case in test/parallel/test-repl-history-navigation.js:

  {
    env: { NODE_REPL_HISTORY: defaultHistoryPath },
    test: [ 'const util = {}', ENTER,
            'ut', RIGHT, ENTER],
    expected: [],
    clean: false
  },

You need to also add a line to make sure the warning is gone at the top of the file:

process.on('warning', common.mustNotCall());

Make sure this fails on current Node.js version, and passes with your PR:

$ ./tools/test.py test/parallel/test-repl-history-navigation.js
[00:00|% 100|+   1|-   0]: Done
$ node test/parallel/test-repl-history-navigation.js || echo 'All good, we expect this to fail'
All good, we expect this to fail

You can add a test case in test/parallel/test-repl-history-navigation.js:

  {
    env: { NODE_REPL_HISTORY: defaultHistoryPath },
    test: [ 'const util = {}', ENTER,
            'ut', RIGHT, ENTER],
    expected: [],
    clean: false
  },

You need to also add a line to make sure the warning is gone at the top of the file:

process.on('warning', common.mustNotCall());

Make sure this fails on current Node.js version, and passes with your PR:

$ ./tools/test.py test/parallel/test-repl-history-navigation.js
[00:00|% 100|+   1|-   0]: Done
$ node test/parallel/test-repl-history-navigation.js || echo 'All good, we expect this to fail'
All good, we expect this to fail

@aduh95 Done! Thanks for the guidance 😄

I... don't understand this fix?

Doesn't this just fix the error being shown (cannot read property length) instead of the underlying cause (the repl assuming util is available in the global namespace)?

I would strongly prefer a fix that fixes the underlying issue and not the "cosmetic" one. Namely changing code doing something like:

session.post('Runtime.callFunctionOn', {
            functionDeclaration: `(v) => util.inspect(v, ${inspectOptions})`,
            objectId: result.objectId,
            arguments: [result]
          }

To not rely on the global util (either by requireing it, "hiding" it as a non-completed global, or passing it to Runtime.evaluate)

To not rely on the global util (either by requireing it, "hiding" it as a non-completed global, or passing it to Runtime.evaluate)

I think it makes sense to change that relevant code from util.inspect to require('util').inspect at least (in addition to the undefined fix, so that doing const require = {} won't break the code). The change would change the "bad" word to require, but I think that it's much less likely to be used.

I think it makes sense to change that relevant code from util.inspect to require('util').inspect at least (in addition to the undefined fix, so that doing const require = {} won't break the code). The change would change the "bad" word to require, but I think that it's much less likely to be used.

That shouldn't be an issue, require is not on the global scope. actually it is, the function still inherits from the REPL scope.

@benjamingr I arrive at the same conclusion as @Linkgoron, we need to rely on either util or require being unchanged –unless there's a way to use dynamic import? Maybe what we can do is to use (v) => Reflect.getOwnPropertyDescriptor(globalThis, 'util').get().inspect(v, ${inspectOptions}), which would fail only if the global util property is re-defined, and wouldn't be "exploitable" unless the user goes all the way of defining a credible mock:

> const util = {}
undefined
> util
{}
> globalThis.util = { inspect() { return console.log(arguments) } }
{ inspect: [Function: inspect] }
> util
{}
> Reflect.defineProperty(globalThis, 'util', { get() { return {inspect(){ console.log(arguments) }}}})
true
> ut[Arguments] {
  '0': {},
  '1': {
    showHidden: false,
    depth: 1,
    colors: false,
    customInspect: true,
    showProxy: true,
    maxArrayLength: 100,
    maxStringLength: 10000,
    breakLength: Infinity,
    compact: true,
    sorted: false,
    getters: false
  }
}

@EladKeyshawn can you please update the code at

@EladKeyshawn can you please update the code at

node/lib/internal/repl/utils.js

Lines 353 to 357 in 7216eb6

	session.post('Runtime.callFunctionOn', {
	functionDeclaration: `(v) => util.inspect(v, ${inspectOptions})`,
	objectId: result.objectId,
	arguments: [result]
	}, (error, preview) => {

with my above suggestion? It would be nice to also add a test case fro when globalThis.util is re-assigned.

No problem :) I'm on it

with my above suggestion? It would be nice to also add a test case fro when globalThis.util is re-assigned.

If globalThis is reassigned we're back with the same problem. Do we count on the fact that it's unlikely to happen? anyways my original patch solves the Cannot read property 'length' of undefined warning in this case anyways.

@aduh95 Can you explain why the last tests pass the expected output but are not marked as called, I believe obviously repl.once('closed'... is not triggered but is that normal?

If globalThis is reassigned we're back with the same problem. Do we count on the fact that it's unlikely to happen?

Right, I think it's fine to fail when globalThis is overwritten, I don't think there is a way around it. I'd say it's possibly less likely to happen than util.

Can you explain why the last tests pass the expected output but are not marked as called, I believe obviously repl.once('closed'... is not triggered but is that normal?

I'm not sure I understand what you mean, I'd expect the closed event to be emitted for each test case. What make you think it's not triggered?

@benjamingr @aduh95 What's blocking this?

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

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

test-repl-history-navigation is consistently failing on ubuntu1804_sharedlibs_withoutintl_x64 and ubuntu1804_sharedlibs_withoutssl_x64 jobs.

test-repl-history-navigation is consistently failing on ubuntu1804_sharedlibs_withoutintl_x64 and ubuntu1804_sharedlibs_withoutssl_x64 jobs.

Do you have any idea what could cause this? for me on OSx it works.

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

Landed in b2baa3d 🎉

I had to revert because of lint failures. I will land this manually.

@benjamingr I have this ready to land on my local machine, do you want me to push?

@aduh95 yes please, thanks :) I want to investigate what's went wrong and I don't want to hold this PR up just for my root-cause check

Landed in d666964 :)

Thanks 😊