Skip to content

Conversation

@jasnell
Copy link
Member

Furthering the cause of breaking node.cc up into smaller, more manageable chunks.

This does two main things:

  1. Adds a new bootstrapper object that is used only during bootstrap to avoid having to delete and overwrite properties on process during bootstrap

  2. Moves most of the process function and property definitions to node_process.cc for better isolation.

Functionality is the same, tho there are some code cleanups along the way. This should be semver-patch.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

jasnell added 3 commits May 21, 2018 19:32
This continues the reorganization of node.cc by splitting the implementations of most process object functions and properties out to a separate node_process.cc and creates a separate temporary bootstrapper object used during Node.js bootstrap to avoid the need for deleting/overwriting process properties set during bootstrap.
Various cleanups within SetupProcessObject to improve readability
@nodejs-github-botnodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 22, 2018
@jasnelljasnell requested a review from addaleaxMay 22, 2018 02:39
@trivikrtrivikr added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 22, 2018
Copy link
Member

@targostargos left a comment

Choose a reason for hiding this comment

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

JS changes LGTM.
Rubber stamp LGTM on C++ changes, assuming it's mostly cut and paste.
Big +1 on reducing the number of lines in node.cc!

@targos
Copy link
Member

@addaleax
Copy link
Member

Can we do this in smaller chunks, more slowly? Landing a patch this size is going to be a major source of conflicts, and not just with things like #20876

@jasnell
Copy link
MemberAuthor

Yeah, splitting it up is possible but will take awhile longer. I'd like to get agreement on this path before staring to do that. :)

@jasnell
Copy link
MemberAuthor

@addaleax ... before I start splitting this up, can I ask you to give me a better sense for how it should be split up so the effort will have the least dramatic impact on what you're working on with the Workers stuff.

@jasnell
Copy link
MemberAuthor

Starting to break things up a bit: #20917

@addaleax
Copy link
Member

@jasnell Yes, that smaller PR looks better with regard to conflicts :)

In general, the easiest thing to check for how much "conflict potential" there is would be applying on PRs on top of the other… taking it in small chunks is definitely the right approach here

@jasnelljasnell added the wip Issues and PRs that are still a work in progress. label May 24, 2018
@jasnelljasnell changed the title src: introduce node_bootstrap and node_process[WIP] src: introduce node_bootstrap and node_processMay 24, 2018
@jasnelljasnell closed this May 30, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.wipIssues and PRs that are still a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jasnell@targos@addaleax@trivikr@nodejs-github-bot