Improve current workflow

[gengjiawen]

The core idea

Introduce Nodejs as base dependency to simplify current workflow.

The problem

• Upgrade toolchain like eslint cumbersome.
• Cpp format is really painful.
  When I work on src: refactor to nullptr #25888. I am pretty painful. With @joyeecheung awesome's work, we have clang-format also with make-formatc-cpp. But it's only available on unix-like os. And we have not clang-format the whole repo. I ended up copy the related snippet.
• Lint fix
  Node community is more familiar with nodejs tools like npm or yarn instead of bash or bat file.
• Use more community tools
  We may need to introduce more tools to improve develop efficiency like prettier and husky.

How do we achieve this

• create package.json in project root and add nodejs in prerequesite.md
• move eslint and other packages to pacakge.json, remove the sources.
• introduce husky and lint-staged, and add clang-format to the hooks. We can see it in electron and repo like CRA see https://github.com/facebook/create-react-app/blob/master/package.json and https://github.com/electron/electron/blob/master/package.json.
  Also we should agree on .clang-format rules and format the src and test folder commit to the repo.

Update on 2019-02-07:

• Not use related toolchain in build phase
• Not introduce husky and lint-staged for now, see Improve current workflow #25908 (comment)
• Not clang-format the whole repo, see [Internal] clang-format the whole repo #25955 (comment)

Update on 2019-02-12

• There are other reason I don't like copy all eslint and babel-eslint and all its dependency code to this repo. It's just a tool. Also with this review eslint upgrade will be much easier.
• Also I think we have same lint logic in both Makefile and vcbuild.bat. Unify it in package.json will remove redundant code.

[devsnek]

Node community is more familiar with nodejs tools like npm or yarn instead of bash or bat file.

This repo is designed so that you can download it once and build or develop node without 1) having an existing install of node or 2) an internet connection

Using npm/yarn would break that.

husky

There is a reason that git doesn't track/automatically install git hooks. Husky is a pretty dangerous tool and I'd be uncomfortable with it being used here.

[gengjiawen]

This repo is designed so that you can download it once and build or develop node without 1) having an existing install of node or 2) an internet connection

Good point, maybe only use it in lint and check steps. Currently some tools in tools folder is already need you have network access.

There is a reason that git doesn't track/automatically install git hooks. Husky is a pretty dangerous tool and I'd be uncomfortable with it being used here.

I think husky is pretty useful. Many repos is using it.

[maxnordlund]

I wholeheartedly agree with @devsnek about avoiding husky. I would be hesitate to contribute if you install git hooks willy-nilly like that. The same goes for other install scripts executed by npm/yarn by default. You can turn it off in yarn by running yarn config set ignore-scripts true like I have.

But I also agree that the node community is arguably more comfortable using tools from npm. Having a package.json, but checking in node_modules or similar, might be a middle ground worth exploring.

Anyway, that's just my two cents coming from the sidelines. Thanks for all the great work you do, really appreciate it ❤️

[joyeecheung]

IMO git hooks do not help with everyone's workflows, especially when they are installed by default as part of the development workflow without a way out. Hooks like pre-commit or pre-push could be helpful when working on smaller changes, for example, PRs with only one patch and only a diff < 100 lines, but they could also get in the way when prototyping bigger changes locally and in personal forks.

[gengjiawen]

Hooks like pre-commit or pre-push could be helpful when working on smaller change

I don't know if that's the case in cpp. But with js project even big pr it will help a lot.
You can see projects like vue, CRA, electron is using it too. Which is quite time saving in my experience.

I see electron use git hook for cpp too: https://github.com/electron/electron/blob/d53b51607c7b69bbc53c41b984963eef21d69f0b/package.json#L79. But maybe not the case for node ? Or we can just use it with js files ?

[joyeecheung]

@gengjiawen This is just me guessing but electron has a dedicated team whose members work at the same company so it would be easier to enforce development practices via things like git hooks. The same applies to v8 and chromium to some extent but even git cl with all its conventions does not use local hooks but instead relies on bots to make sure the tree is in shape. Node does not, however, have a dedicated team like that, and different contributors have very different styles of working. I know some contributors, including me, like prototyping changes incrementally via "malformed" commits and pushing them to personal forks before cleaning them all up, writing proper commit messages, and opening a proper PR. With that style of working git hooks that yell at you on malformed commit messages or style nits would be counter-productive.

[gengjiawen]

I have update some info in original post.
Also I want to make some try on this:

• create package.json in project root
• refactor eslint to dependency. (Prefer internal npm to install ?)
• replace lint task in bash and bat to npm task.

[joyeecheung]

If we are going to use npm to install the development tools, we could just put all that into https://github.com/nodejs/node-core-utils and implement something like git node presubmit that performs whatever checks necessary for a proper PR (similar to git cl presubmit). Then we can just separate development tools from build tools and remove all the packages out of this repo.

(But I am not sure if people are going to like the idea of having to npm install node-core-utils before trying to start any contribution, not everybody is very fond of chromium's depot_tools, for example. Also this means that the formatter versions are no longer tracked in this repo which could result in some complexities if people do not remember to keep their node-core-utils installation up-to-date)

[gengjiawen]

How current npm lib are installed in tools folder like clang-format ?

Update on 19/02/11:
found it:

node/Makefile

Lines 1238 to 1239 in 4deb23a

	format-cpp-build:
	cd tools/clang-format && $(call available-node,$(run-npm-ci))

[jasnell]

Closing this as there's been no further activity. @gengjiawen , if this is something you still believe we should pursue then it can be reopened.