lib,src: remove vm.runInDebugContext() #13295

bnoordhuis · 2017-05-30T12:38:16Z

The V8 API it is based on is deprecated and scheduled for removal later this year. Remove it.

The first commit replaces the Debug.MakeMirror() call in lib/util.js with something else.

The most important reason for doing this is of course to have a deprecation warning-free build again!

addaleax · 2017-05-30T12:40:24Z

I think @joshgav or @ofrobots had the plan of removing this after Node 9 (i.e. in Node 10)?

bnoordhuis · 2017-05-30T12:40:03Z

.eslintignore

@@ -1,3 +1,4 @@
+lib/internal/v8.js


The file is whitelisted because the V8 builtins don't lint. They're not legal JS syntax, eslint can't parse them.

You should be able add comments by starting lines with # – if you comment here in the PR, that sounds like a good indicator that it makes sense to do so. :)

There are other lib/internal/v8 files whitelisted. I don't think we need a comment. But either way is fine.

bnoordhuis · 2017-05-30T12:44:53Z

lib/util.js

-  const mirror = Debug.MakeMirror(value, true);
+function formatMapIterator(ctx, value, recurseTimes, visibleKeys, keys) {
+  var nextRecurseTimes = recurseTimes === null ? null : recurseTimes - 1;
+  var vals = previewMapIterator(value, 100);


There is a minor change in behavior here in that the preview is now limited to 100 elements. Before, it would show them all but that is (IMO) rather annoying with large maps and sets (and it's basically equivalent to printing the map or set minus the already iterated over elements.)

I'm planning to follow up with a PR that prints a nice "..." if there are more elements but I can restore the old behavior if people prefer that.

bnoordhuis · 2017-05-30T12:49:03Z

I think @joshgav or @ofrobots had the plan of removing this after Node 9 (i.e. in Node 10)?

In a LTS release? That doesn't make sense. Removing it in node 9 gives people the chance to update their code bases before entering the next LTS.

targos · 2017-05-30T13:06:31Z

Documentation says Node 10+: https://github.com/nodejs/node/blob/master/doc/api/deprecations.md#dep0069-vmrunindebugcontextstring

According to our deprecation policy, we should first emit a warning in Node 9 and remove the feature in Node 10 or higher. I guess it would have to be 10 here because V8 will remove the API by the end of the year.

addaleax · 2017-05-30T13:13:42Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/845/

I don’t know about the reasons behind not going for a runtime deprecation in Node 8, I was just pretty sure I read somewhere that it was intentional. ¯\_(ツ)_/¯

targos · 2017-05-30T13:23:11Z

Most of the discussion is here: #12243
PR for the runtime deprecation: #12815

addaleax · 2017-05-30T13:21:10Z

.eslintignore

@@ -1,3 +1,4 @@
+lib/internal/v8.js


You should be able add comments by starting lines with # – if you comment here in the PR, that sounds like a good indicator that it makes sense to do so. :)

addaleax · 2017-05-30T13:22:10Z

doc/api/vm.md

-added: v0.11.14
-->
-
-> Stability: 0 - Deprecated. An alternative is in development.


You’ll also want to update the entry in deprecations.md to say Type: End-of-Life

bnoordhuis · 2017-05-30T13:31:35Z

According to our deprecation policy, we should first emit a warning in Node 9 and remove the feature in Node 10 or higher.

That doesn't apply to vm.runInDebugContext(), its documentation has always stated very clearly that it could disappear at any time.

jasnell · 2017-05-30T14:19:22Z

We really ought to have had a deprecation message added in 8.0.0 for this but it's rather too late now. If the plan is to remove it in 9.0, then we should get a runtime deprecation into an 8.x minor.

TimothyGu

For context: the first part of this PR which removes the use of Debug Mirrors is, well, hacky as heck. But there is no other solution as I have found out when investigating #11875, other than convincing V8 devs to add C++ APIs for Map- and SetIterators.

@bnoordhuis can you attach a Fixes: https://github.com/nodejs/node/issues/11875 to the commit message as well?

TimothyGu · 2017-05-31T23:43:50Z

lib/internal/v8.js

+  while (n-- > 0) {
+    const rc = %MapIteratorNext(it, out);
+    if (rc === 0) break;
+    if (rc === 1) result.push(out[0]);       // it.keys()


Can the 0/1/2/3 magic numbers be constants instead? V8 is using

define ITERATOR_KIND_KEYS = 1; define ITERATOR_KIND_VALUES = 2; define ITERATOR_KIND_ENTRIES = 3;

which looks good to me.

TimothyGu · 2017-05-31T23:44:50Z

lib/internal/v8.js

+  it = %MapIteratorClone(it);
+  const result = [];
+  const out = [,,];
+  while (n-- > 0) {


Can a for-of loop be used directly on the cloned it?

fhinkel

LGTM with nit

fhinkel · 2017-08-03T11:50:56Z

.eslintignore

@@ -1,3 +1,4 @@
+lib/internal/v8.js


There are other lib/internal/v8 files whitelisted. I don't think we need a comment. But either way is fine.

fhinkel · 2017-08-03T11:59:19Z

test/parallel/test-util-inspect.js

 'use strict';
 const common = require('../common');
 const assert = require('assert');
 const util = require('util');
 const vm = require('vm');
+const { previewMapIterator } = require('internal/v8');


Can we test the SetIterator as well?

I didn't add a test here, I just updated an existing test that was using internals.

TimothyGu · 2017-08-03T13:37:41Z

@fhinkel Do you have a better way to preserve the Iterator support w/o using V8 natives syntax?

fhinkel · 2017-08-04T06:43:02Z

Maybe we could add these methods to the V8 API. Relying on intrinsic functions (%) is somewhat dangerous. They are not part of the API and we might change/delete them without a deprecation period.

bnoordhuis · 2017-08-04T06:57:06Z

Yes, that's my longer-term goal. Using natives syntax is a short-term + least-effort solution.

BridgeAR · 2017-08-30T19:12:25Z

This needs a rebase.

TimothyGu

LGTM once #13295 (comment) is addressed.

BridgeAR · 2017-09-19T22:35:33Z

@bnoordhuis do you still want to follow up on this?

bnoordhuis · 2017-10-02T09:21:22Z

Updated. CI for a change: https://ci.nodejs.org/job/node-test-pull-request/10379/

ofrobots · 2017-10-16T22:31:02Z

I would like to formally request that we do NOT land this in Node 9. We have been migrating some of the code we use in https://github.com/GoogleCloudPlatform/cloud-debug-nodejs to the inspector module. At this point we do not think the inspector API is a complete replacement for the Debug context, at least for our use cases. See V8 bug: https://bugs.chromium.org/p/v8/issues/detail?id=6945.

Since there is no pressing need to remove the API just yet, let's not accelerate the removal.

mcollina · 2017-10-17T07:52:54Z

👍 in waiting.

addaleax · 2017-10-23T15:24:54Z

This would need to be rebased now that the runtime deprecation has been merged.

BridgeAR · 2017-11-22T11:45:07Z

Ping @bnoordhuis this could land after a rebase

bnoordhuis · 2017-11-22T13:19:25Z

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

targos · 2017-11-22T13:36:35Z

tools/doc/node_modules/js-yaml/index.js

@@ -1,15 +1,7 @@
 'use strict';

-// Hack to load the js-yaml module from eslint.


unrelated change?

grr... this one is becoming quite annoying.

jasnell · 2017-11-22T18:27:15Z

@bnoordhuis ... the extraneous js-yaml change is causing CI to fail on this.

bnoordhuis · 2017-11-23T20:35:40Z

With js-yaml change undone: https://ci.nodejs.org/job/node-test-pull-request/11679/

This paves the way for removing `vm.runInDebugContext()`. Inspection of Map and Set iterators is now done through V8 instrinsics. Fixes: nodejs#11875 PR-URL: nodejs#13295 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

The V8 API it is based on is deprecated and scheduled for removal later this year. Remove it. PR-URL: nodejs#13295 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

addaleax · 2017-12-04T20:57:54Z

Btw: This breaks gulp because it uses graceful-fs@3.x and their parsing of process.binding('natives') doesn’t expect the --allow_natives_syntax cleverness.

That’s nothing we can do anything about, we need this patch in order to avoid breakage ourselves, so it’s graceful-fs that needs to work around this, but I wanted to leave that information here.

The V8 API it is based on is deprecated and scheduled for removal later this year. Remove it. PR-URL: nodejs#13295 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Robert Jefe Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

bnoordhuis added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 30, 2017

bnoordhuis commented May 30, 2017

View reviewed changes

addaleax approved these changes May 30, 2017

View reviewed changes

TimothyGu reviewed May 31, 2017

View reviewed changes

refack mentioned this pull request Jul 14, 2017

[wip] src, inspector: support opted-in VM contexts #14231

Closed

4 tasks

bnoordhuis mentioned this pull request Jul 30, 2017

Find an alternative to V8's Debug Mirror API for util.inspect #11875

Closed

fhinkel approved these changes Aug 3, 2017

View reviewed changes

fhinkel mentioned this pull request Aug 3, 2017

src: deprecate vm.runInDebugContext #12815

Closed

4 tasks

jasnell approved these changes Aug 30, 2017

View reviewed changes

TimothyGu approved these changes Aug 31, 2017

View reviewed changes

eljefedelrodeodeljefe approved these changes Sep 4, 2017

View reviewed changes

BridgeAR added stalled Issues and PRs that are stalled. and removed stalled Issues and PRs that are stalled. labels Sep 8, 2017

BridgeAR approved these changes Sep 19, 2017

View reviewed changes

addaleax added this to the 10.0.0 milestone Oct 22, 2017

bnoordhuis force-pushed the remove-runindebugcontext branch from ef78a22 to fce7077 Compare November 22, 2017 13:17

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 22, 2017

targos reviewed Nov 22, 2017

View reviewed changes

bnoordhuis force-pushed the remove-runindebugcontext branch from fce7077 to 76796b7 Compare November 23, 2017 20:34

bnoordhuis added 2 commits November 24, 2017 00:13

bnoordhuis force-pushed the remove-runindebugcontext branch from 76796b7 to 6f724e1 Compare November 23, 2017 23:16

bnoordhuis closed this Nov 23, 2017

bnoordhuis deleted the remove-runindebugcontext branch November 23, 2017 23:16

bnoordhuis merged commit 6f724e1 into nodejs:master Nov 23, 2017

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017

addaleax mentioned this pull request Dec 5, 2017

Failures on master nodejs/citgm#518

Closed

This was referenced Dec 5, 2017

v3.x branch Broken on Node.js master isaacs/node-graceful-fs#120

Closed

Broken on Node.js 8, 9, and Master isaacs/natives#14

Closed

bnoordhuis mentioned this pull request Feb 8, 2018

v8::debug::GetDebugContext is going away. #18643

Closed

		@@ -1,15 +1,7 @@
		'use strict';

		// Hack to load the js-yaml module from eslint.

lib,src: remove vm.runInDebugContext() #13295

lib,src: remove vm.runInDebugContext() #13295

Conversation

bnoordhuis commented May 30, 2017

addaleax commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented May 30, 2017

targos commented May 30, 2017

addaleax commented May 30, 2017

targos commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented May 30, 2017

jasnell commented May 30, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhinkel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Aug 3, 2017

fhinkel commented Aug 4, 2017

bnoordhuis commented Aug 4, 2017

BridgeAR commented Aug 30, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

BridgeAR commented Sep 19, 2017

bnoordhuis commented Oct 2, 2017

ofrobots commented Oct 16, 2017

mcollina commented Oct 17, 2017

addaleax commented Oct 23, 2017

BridgeAR commented Nov 22, 2017

bnoordhuis commented Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Nov 22, 2017

bnoordhuis commented Nov 23, 2017

addaleax commented Dec 4, 2017

bnoordhuis commented Nov 22, 2017 •

edited

Loading