Repository navigation
Improve current workflow #25908
Description
Activity
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.
Reacted by ZYSzys, Max Nordlund and mary marchiniThis 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
toolsfolder 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.
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
installscripts executed by npm/yarn by default. You can turn it off inyarnby runningyarn config set ignore-scripts truelike 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 ❤️
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.
Reacted by snek and mary marchiniHooks 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 ?
@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 clwith 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.Reacted by Max Nordlund, mary marchini and David CorbachoI have update some info in original post.
Also I want to make some try on this:- create
package.jsonin project root - refactor eslint to dependency. (Prefer internal npm to install ?)
- replace lint task in bash and bat to npm task.
- create
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 presubmitthat performs whatever checks necessary for a proper PR (similar togit 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-utilsbefore trying to start any contribution, not everybody is very fond of chromium'sdepot_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 theirnode-core-utilsinstallation up-to-date)How current npm lib are installed in tools folder like clang-format ?
Update on 19/02/11:
found it:
Lines 1238 to 1239 in 4deb23a
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.
The core idea
Introduce Nodejs as base dependency to simplify current workflow.
The problem
When I work on src: refactor to nullptr #25888. I am pretty painful. With @joyeecheung awesome's work, we have
clang-formatalso withmake-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.Node community is more familiar with nodejs tools like npm or yarn instead of bash or bat file.
We may need to introduce more tools to improve develop efficiency like
prettierandhusky.How do we achieve this
package.jsonin project root and add nodejs inprerequesite.mdpacakge.json, remove the sources.huskyandlint-staged, and addclang-formatto the hooks. We can see it in electron and repo likeCRAsee 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-formatrules and format thesrcandtestfolder commit to the repo.Update on 2019-02-07:
huskyandlint-stagedfor now, see Improve current workflow #25908 (comment)Update on 2019-02-12
eslintandbabel-eslintand all its dependency code to this repo. It's just a tool. Also with this review eslint upgrade will be much easier.Makefileandvcbuild.bat. Unify it inpackage.jsonwill remove redundant code.