change for a deprecated method: SetNamedPropertyHandler() to SetHandler() #9062

AnnaMag · 2016-10-12T18:24:38Z

Checklist

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

Description of change

SetNamedPropertyHandler() method replaced with SetHandler() in process_env_template

PropertyHandlerFlags::kOnlyInterceptStrings flag maintains previous behavior: only Strings as properties are intercepted. In theory, it is now possible to extend the functionality for intercepting properties using Symbols (in practice a fix is required: I will create an issue when this pull request is merged).

cjihrig · 2016-10-12T18:40:17Z

test/parallel/test-v8-interceptStrings-not-Symbols.js

+const assert = require('assert');
+
+// Note (@AnnaMag): test for unchanged behaviour after refactoring to SetHandler
+//(see: node.cc line 3158)


Can you remove the references to specific lines. They will almost certainly get stale quickly.

cjihrig · 2016-10-12T18:41:55Z

test/parallel/test-v8-interceptStrings-not-Symbols.js

+// Note (@AnnaMag): test for unchanged behaviour after refactoring to SetHandler
+//(see: node.cc line 3158)
+//
+// with the kOnlyInterceptStrings flag, manipulating properties via


Can you capitalize and punctuate the the comments. Also, add a space after // in some of the comments below.

cjihrig · 2016-10-12T18:43:40Z

test/parallel/test-v8-interceptStrings-not-Symbols.js

+assert.strictEqual('42', process.env['s']);
+
+//check the type of sustituted properties
+assert.strictEqual('string', typeof process.env['s'])


These assertions are already covered by the previous two assertions.

bnoordhuis

LGTM with a suggestion.

bnoordhuis · 2016-10-13T11:45:32Z

test/parallel/test-v8-interceptStrings-not-Symbols.js

+// (node::Utf8Value key(info.GetIsolate(), property);).
+
+
+var symbol = Symbol('sym');


bnoordhuis · 2016-10-13T11:46:53Z

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

jasnell

LGTM

fhinkel · 2016-10-13T20:07:52Z

Can you squash the commits into one?

For the comments in the tests, right now some of them document what you did in the refactoring. For somebody reading the tests in a few month, it's probably not so important that there was a refactoring, but more what the test is testing. Maybe you can rewrite the comments not so much focusing on your change, but rather what you want to test (The test is valid with or without the refactoring).

cjihrig

LGTM, though I agree with @fhinkel about the comments. You also don't need to include your handle in the comment.

AnnaMag · 2016-10-14T13:16:15Z

Done.

jasnell · 2016-10-14T16:10:37Z

@AnnaMag ... can you please update the commit log to match the contribution guidelines here: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit

jasnell · 2016-10-19T15:54:59Z

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

AnnaMag · 2016-10-26T18:45:53Z

@jasnell, the failures are timeouts, which in my understanding are not patch related. Am I right or missing sth?

jasnell · 2016-10-26T18:54:33Z

Yep, they appear to be unrelated

fhinkel · 2016-10-27T08:31:55Z

Can you address Ben's comment? Then we can merge this.

The changes introdcued here replace the deprecated v8 method SetNamedPropertyHandler() to SetHandler() in node.cc. Prior to refactoring, the method defined callbacks when accessing object properties defined by Strings and not Symbols. test/parallel/test-v8-interceptStrings-not-Symbols.js demonstrates that this behaviour remained unchanged after refactoring.

AnnaMag · 2016-10-27T11:12:47Z

Done:)

fhinkel · 2016-11-01T18:58:50Z

LGTM

fhinkel · 2016-11-01T19:23:31Z

Landed in ab19412

The changes introdcued here replace the deprecated v8 method SetNamedPropertyHandler() to SetHandler() in node.cc. Prior to refactoring, the method defined callbacks when accessing object properties defined by Strings and not Symbols. test/parallel/test-v8-interceptStrings-not-Symbols.js demonstrates that this behaviour remained unchanged after refactoring. PR-URL: #9062 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

MylesBorins · 2016-11-22T07:28:35Z

should this be backported?

fhinkel · 2016-11-22T09:28:26Z

@thealphanerd Live quoting you: "If it lands cleanly, yes please". Thanks.

The changes introdcued here replace the deprecated v8 method SetNamedPropertyHandler() to SetHandler() in node.cc. Prior to refactoring, the method defined callbacks when accessing object properties defined by Strings and not Symbols. test/parallel/test-v8-interceptStrings-not-Symbols.js demonstrates that this behaviour remained unchanged after refactoring. PR-URL: #9062 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

MylesBorins · 2016-11-26T08:25:38Z

@fhinkel landed cleanly on v6 but not v4, please feel free to manually backport

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 12, 2016

cjihrig reviewed Oct 12, 2016

View reviewed changes

mscdex added the process Issues and PRs related to the process subsystem. label Oct 12, 2016

bnoordhuis approved these changes Oct 13, 2016

View reviewed changes

AnnaMag force-pushed the sethandler branch from 11f1940 to a877fa8 Compare October 13, 2016 13:44

jasnell approved these changes Oct 13, 2016

View reviewed changes

cjihrig approved these changes Oct 13, 2016

View reviewed changes

AnnaMag force-pushed the sethandler branch from 9a51ba6 to de654a5 Compare October 14, 2016 13:14

AnnaMag force-pushed the sethandler branch from de654a5 to 2e5c69d Compare October 14, 2016 13:25

AnnaMag force-pushed the sethandler branch 4 times, most recently from 3bc11a4 to 8b404de Compare October 14, 2016 19:27

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

AnnaMag force-pushed the sethandler branch from 1ae0fe1 to 9c397a0 Compare October 27, 2016 11:11

fhinkel closed this Nov 1, 2016

evanlucas mentioned this pull request Nov 3, 2016

v7.1.0 proposal - 2016-11-08 #9438

Merged

MylesBorins added lts-watch-v4.x labels Nov 22, 2016

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Nov 26, 2016

MylesBorins mentioned this pull request Dec 1, 2016

v6.9.2 proposal #9735

Merged

AnnaMag deleted the sethandler branch January 25, 2017 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change for a deprecated method: SetNamedPropertyHandler() to SetHandler() #9062

change for a deprecated method: SetNamedPropertyHandler() to SetHandler() #9062

AnnaMag commented Oct 12, 2016 •

edited

Loading

cjihrig Oct 12, 2016

cjihrig Oct 12, 2016

cjihrig Oct 12, 2016

bnoordhuis left a comment

bnoordhuis Oct 13, 2016

bnoordhuis commented Oct 13, 2016

jasnell left a comment

fhinkel commented Oct 13, 2016

cjihrig left a comment

AnnaMag commented Oct 14, 2016

jasnell commented Oct 14, 2016

jasnell commented Oct 19, 2016

AnnaMag commented Oct 26, 2016

jasnell commented Oct 26, 2016

fhinkel commented Oct 27, 2016

AnnaMag commented Oct 27, 2016

fhinkel commented Nov 1, 2016

fhinkel commented Nov 1, 2016

MylesBorins commented Nov 22, 2016

fhinkel commented Nov 22, 2016

MylesBorins commented Nov 26, 2016

		// (node::Utf8Value key(info.GetIsolate(), property);).


		var symbol = Symbol('sym');

change for a deprecated method: SetNamedPropertyHandler() to SetHandler() #9062

change for a deprecated method: SetNamedPropertyHandler() to SetHandler() #9062

Conversation

AnnaMag commented Oct 12, 2016 • edited Loading

Checklist

Description of change

cjihrig Oct 12, 2016

Choose a reason for hiding this comment

cjihrig Oct 12, 2016

Choose a reason for hiding this comment

cjihrig Oct 12, 2016

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Oct 13, 2016

Choose a reason for hiding this comment

bnoordhuis commented Oct 13, 2016

jasnell left a comment

Choose a reason for hiding this comment

fhinkel commented Oct 13, 2016

cjihrig left a comment

Choose a reason for hiding this comment

AnnaMag commented Oct 14, 2016

jasnell commented Oct 14, 2016

jasnell commented Oct 19, 2016

AnnaMag commented Oct 26, 2016

jasnell commented Oct 26, 2016

fhinkel commented Oct 27, 2016

AnnaMag commented Oct 27, 2016

fhinkel commented Nov 1, 2016

fhinkel commented Nov 1, 2016

MylesBorins commented Nov 22, 2016

fhinkel commented Nov 22, 2016

MylesBorins commented Nov 26, 2016

AnnaMag commented Oct 12, 2016 •

edited

Loading