Migrating from process.binding('config') to getOptions() #23588

burgerboydaddy · Oct 12, 2018

Change inside /lib/internal/bootstrap/node.js code from
process.binding('config') to
internalBinding('options).getOptions()

Changes are done for options:
--experimental-modules
--experimental-vm-modules
--experimental-worker

Checklist

[x ] make -j4 test (UNIX), or vcbuild test (Windows) passes

jasnell · Oct 12, 2018

jasnell approved these changes Oct 12, 2018

View changes

devsnek · Oct 12, 2018

devsnek reviewed Oct 12, 2018

View changes

devsnek · Oct 12, 2018

lib/internal/bootstrap/node.js

@@ -137,7 +137,7 @@
      setupQueueMicrotask();
    }

-    if (process.binding('config').experimentalWorker) {
+    if (internalBinding('options').getOptions('--experimental-worker')) {


can you factor out the internalBinding into a single call up top and also save the flag calls as variables to be used later?

something like

const { getOptions } = internalBinding('options'); const experimentalModules = getOptions('experimentalModules'); // ... if (experimentalModules) { // ...

Yes I will do that.
But one question. There is already some similar code in place, like line 114:

const options = internalBinding('options'); if (options.getOptions('--help')) { NativeModule.require('internal/print_help').print(process.stdout); return; }

Should I redo that part also and change code to use as mentioned:

const { getOptions } = internalBinding('options'); const experimentalModules = getOptions('experimentalModules'); // ... if (experimentalModules) { // ...

Thanks

As mentioned in my previous comment, will code like this (replacing line 114) be Ok:

const { getOptions } = internalBinding('options'); const helpOption = getOptions('--help'); const completionBashOption = getOptions('--completion-bash'); const experimentalModulesOption = getOptions('--experimental-modules'); const experimentalVMModulesOption = getOptions('--experimental-vm-modules'); const experimentalWorkerOption = getOptions('--experimental-worker');

@burgerboydaddy Yes, that definitely looks okay :)

gireeshpunathil · Oct 13, 2018

gireeshpunathil approved these changes Oct 13, 2018

View changes

approving with @devsnek 's suggestion

thefourtheye · Oct 13, 2018

thefourtheye approved these changes Oct 13, 2018

View changes

LGTM with @devsnek's suggestion.

burgerboydaddy · Oct 17, 2018

Added suggested changes to the code

const { getOptions } = internalBinding('options');
    const helpOption = getOptions('--help');
    const completionBashOption = getOptions('--completion-bash');
    const experimentalModulesOption = getOptions('--experimental-modules');
    const experimentalVMModulesOption = getOptions('--experimental-vm-modules');
    const experimentalWorkerOption = getOptions('--experimental-worker');

Please review.

addaleax · Oct 17, 2018

addaleax reviewed Oct 17, 2018

View changes

addaleax · Oct 17, 2018

lib/internal/bootstrap/node.js

@@ -183,7 +188,7 @@
    {
      // Install legacy getters on the `util` binding for typechecking.
      // TODO(addaleax): Turn into a full runtime deprecation.
-      const { pendingDeprecation } = process.binding('config');
+      const { pendingDeprecation } = internalBinding('options').getOptions();


@burgerboydaddy I think something went wrong with this one?

@addaleax initial request was to replace process.binding('config') to
internalBinding('options).getOptions(). Is there any reason why this line shouldn't be included?
If yes, I will restore this line to original one.
Please let me know what is your opinion.

thanks

burgerboydaddy · Oct 19, 2018

Fixed issues.
returned process.binding('config'); in line 191:

-      const { pendingDeprecation } = internalBinding('options').getOptions();
+      const { pendingDeprecation } = process.binding('config');

trivikr · Oct 20, 2018

trivikr approved these changes Oct 20, 2018

View changes

addaleax · Oct 21, 2018

@burgerboydaddy Something seems to be odd here … github says there are about 200 commits in this PR, some of which are merge commits?

Do you think you could rebase this against master (not merge)?

burgerboydaddy · Oct 21, 2018

@addaleax I'm even thinking to re-do complete request (new PR). Will that be better?

addaleax · Oct 21, 2018

@burgerboydaddy You can do that, but rebasing this one should be totally fine as well – whichever you prefer

burgerboydaddy · Oct 21, 2018

@burgerboydaddy You can do that, but rebasing this one should be totally fine as well – whichever you prefer

@addaleax This are command that I just executed:

$ git fetch --all
$ git rebase origin/master

During rebase got error:

Using index info to reconstruct a base tree...
M	lib/internal/bootstrap/node.js
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: Updated code for Migrating from process.binding('config') to getOptions() #23588
Applying: Migrating from process.binding('config') to getOptions
Using index info to reconstruct a base tree...
M	lib/internal/bootstrap/node.js
Falling back to patching base and 3-way merge...
Auto-merging lib/internal/bootstrap/node.js
CONFLICT (content): Merge conflict in lib/internal/bootstrap/node.js
error: Failed to merge in the changes.
Patch failed at 0193 Migrating from process.binding('config') to getOptions
Use 'git am --show-current-patch' to see the failed patch

Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".

I fixed issue with file that I changed (node.js). After that did:

git add lib/internal/bootstrap/node.js
git rebase --continue

But after rebase --continue received response:

Applying: Migrating from process.binding('config') to getOptions
No changes - did you forget to use 'git add'?
If there is nothing left to stage, chances are that something else
already introduced the same changes; you might want to skip this patch.

What should I do?

addaleax · Oct 21, 2018

@burgerboydaddy That’s odd … it would typically mean that the changes from your commits have already happened upstream; but that’s clearly not the case, so I’m not sure what to do with that.

burgerboydaddy · Oct 21, 2018

@addaleax ok, how about to close this PR and create new one with proper code? I will mention this id (#23588) in my comments?

Trott · Oct 22, 2018

Rebased, addressed merge conflict, force pushed.

Trott · Oct 22, 2018

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

Trott · Oct 23, 2018

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/18063/

Trott · Oct 23, 2018

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/18080/

Trott · Oct 24, 2018

Just landed a PR to fix the unreliable test that failed last CI run...

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/18094/

Trott · Oct 24, 2018

Landed in bb79e76.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

targos added the code-and-learn label Oct 12, 2018

burgerboydaddy force-pushed the burgerboydaddy:node-contrib1 branch from 678e0f4 to 7d85fb6 Oct 17, 2018

burgerboydaddy added a commit to burgerboydaddy/node that referenced this pull request Oct 17, 2018

Updated code for Migrating from process.binding('config') to getOptio…
…ns() nodejs#23588

14d94b4

burgerboydaddy added a commit to burgerboydaddy/node that referenced this pull request Oct 17, 2018

Updated code for Migrating from process.binding('config') to getOptio…
…ns() nodejs#23588

986052b

burgerboydaddy added a commit to burgerboydaddy/node that referenced this pull request Oct 17, 2018

update - Migrating from process.binding('config') to getOptions() nod…
…ejs#23588

afbdd64

burgerboydaddy added some commits Oct 12, 2018

Migrating from process.binding('config') to getOptions

61b495a

Fixing found issues

Loading status checks…

7636912

Trott force-pushed the burgerboydaddy:node-contrib1 branch from 5a49556 to 7636912 Oct 22, 2018

Trott added some commits Oct 22, 2018

fixup! Fixing found issues

Loading status checks…

371d0bd

fixup! fixup! Fixing found issues

Loading status checks…

fa92ae4

Trott closed this Oct 24, 2018

targos referenced this pull request Oct 27, 2018
Merged
Release proposal: v11.1.0 #23922

MylesBorins added dont-land-on-v8.x land-on-v10.x labels Nov 26, 2018

codebytere referenced this pull request Nov 27, 2018
Closed
v10.13.1 proposal #24675

codebytere referenced this pull request Nov 29, 2018
Merged
v10.14.2 proposal #24727

nodejs/node

Join GitHub today

Migrating from process.binding('config') to getOptions() #23588

Conversation

burgerboydaddy commented Oct 12, 2018

Checklist

targos added the code-and-learn label Oct 12, 2018

jasnell approved these changes Oct 12, 2018 View changes

devsnek reviewed Oct 12, 2018 View changes

This comment has been minimized.

This comment has been minimized.

burgerboydaddy Oct 14, 2018 • edited

This comment has been minimized.

This comment has been minimized.

gireeshpunathil approved these changes Oct 13, 2018 View changes

gireeshpunathil left a comment

thefourtheye approved these changes Oct 13, 2018 View changes

thefourtheye left a comment

burgerboydaddy force-pushed the burgerboydaddy:node-contrib1 branch from 678e0f4 to 7d85fb6 Oct 17, 2018

burgerboydaddy added a commit to burgerboydaddy/node that referenced this pull request Oct 17, 2018

burgerboydaddy added a commit to burgerboydaddy/node that referenced this pull request Oct 17, 2018

burgerboydaddy added a commit to burgerboydaddy/node that referenced this pull request Oct 17, 2018

This comment has been minimized.

burgerboydaddy commented Oct 17, 2018 • edited

addaleax reviewed Oct 17, 2018 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

burgerboydaddy commented Oct 19, 2018 • edited

trivikr approved these changes Oct 20, 2018 View changes

This comment has been minimized.

addaleax commented Oct 21, 2018

This comment has been minimized.

burgerboydaddy commented Oct 21, 2018

This comment has been minimized.

addaleax commented Oct 21, 2018

This comment has been minimized.

burgerboydaddy commented Oct 21, 2018

This comment has been minimized.

addaleax commented Oct 21, 2018

This comment has been minimized.

burgerboydaddy commented Oct 21, 2018

burgerboydaddy added some commits Oct 12, 2018

Trott force-pushed the burgerboydaddy:node-contrib1 branch from 5a49556 to 7636912 Oct 22, 2018

This comment has been minimized.

Trott commented Oct 22, 2018

Trott added some commits Oct 22, 2018

This comment has been minimized.

Trott commented Oct 22, 2018

This comment has been minimized.

Trott commented Oct 23, 2018

This comment has been minimized.

Trott commented Oct 23, 2018

This comment has been minimized.

Trott commented Oct 24, 2018

Trott added a commit to Trott/io.js that referenced this pull request Oct 24, 2018

This comment has been minimized.

Trott commented Oct 24, 2018

Trott closed this Oct 24, 2018

targos added a commit that referenced this pull request Oct 24, 2018

targos referenced this pull request Oct 27, 2018

This was referenced Nov 2, 2018

MylesBorins added a commit that referenced this pull request Nov 26, 2018

MylesBorins added dont-land-on-v8.x land-on-v10.x labels Nov 26, 2018

codebytere referenced this pull request Nov 27, 2018

rvagg added a commit that referenced this pull request Nov 28, 2018

MylesBorins added a commit that referenced this pull request Nov 29, 2018

codebytere referenced this pull request Nov 29, 2018

jasnell approved these changes Oct 12, 2018

View changes

devsnek reviewed Oct 12, 2018

View changes

burgerboydaddy Oct 14, 2018 •

edited

gireeshpunathil approved these changes Oct 13, 2018

View changes

thefourtheye approved these changes Oct 13, 2018

View changes

burgerboydaddy force-pushed the burgerboydaddy:node-contrib1 branch from `678e0f4` to `7d85fb6` Oct 17, 2018

burgerboydaddy commented Oct 17, 2018 •

edited

addaleax reviewed Oct 17, 2018

View changes

burgerboydaddy commented Oct 19, 2018 •

edited

trivikr approved these changes Oct 20, 2018

View changes

Trott force-pushed the burgerboydaddy:node-contrib1 branch from `5a49556` to `7636912` Oct 22, 2018