Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from

Conversation

burgerboydaddy
Copy link
Contributor

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

@targos targos added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 12, 2018
@@ -137,7 +137,7 @@
setupQueueMicrotask();
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
  // ...

Copy link
Contributor Author

@burgerboydaddy burgerboydaddy Oct 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@burgerboydaddy Yes, that definitely looks okay :)

Copy link
Member

@gireeshpunathil gireeshpunathil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving with @devsnek 's suggestion

Copy link
Contributor

@thefourtheye thefourtheye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with @devsnek's suggestion.

@burgerboydaddy
Copy link
Contributor Author

burgerboydaddy commented 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.

@@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@burgerboydaddy I think something went wrong with this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor Author

burgerboydaddy commented Oct 19, 2018

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

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

@addaleax
Copy link
Member

@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
Copy link
Contributor Author

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

@addaleax
Copy link
Member

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

@burgerboydaddy
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

@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
Copy link
Member

Trott commented Oct 22, 2018

Rebased, addressed merge conflict, force pushed.

@Trott
Copy link
Member

Trott commented Oct 22, 2018

@Trott
Copy link
Member

Trott commented Oct 23, 2018

@Trott
Copy link
Member

Trott commented Oct 23, 2018

@Trott
Copy link
Member

Trott commented 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 pushed a commit to Trott/io.js that referenced this pull request Oct 24, 2018
PR-URL: nodejs#23588
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@Trott
Copy link
Member

Trott commented 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/.)

@Trott Trott closed this Oct 24, 2018
targos pushed a commit that referenced this pull request Oct 24, 2018
PR-URL: #23588
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
PR-URL: #23588
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #23588
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #23588
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants