search

Automattic / a8c-ci-toolkit-buildkite-plugin

A caching plugin that can be invoked from your build script.
21 stars 5 forks source link

Add a script to install node packages using npm #31

Closed rynaardb closed 1 year ago

rynaardb commented 2 years ago

What it does

Add a script to install Node JS packages using npm.

How to test

Who

@Automattic/apps-infrastructure

rynaardb commented 2 years ago

What's the benefit to using this over just running npm install?

I am glad you are asking 😄 I kind of asked the same question, but in a different way on Slack earlier today:

Is this the approach we want to follow “as a rule” for repetitive commands like these, to add them to this plugin?

From my perspective, I also don't see any real benefit other than maybe having a more verbose command name: npm install vs. install_node_packages, and maybe some consistency should we ever decide to use something else other than npm? If none of this matters then I would say there is probably no real benefit and we can go ahead and close this PR.

jkmassel commented 2 years ago

In this repo are some other examples of invoking a package manager – for instance install_gems will transparently take care of caching for us. That would add some benefit for us.

One other suggestion while I'm at it – in this case it seems we're invoking npm, so we should probably call that out. We use install_$foo for out command name, so it makes sense to have install_npm_packages – I could see us also having an install_yarn_packages command to invoke that package manager.

rynaardb commented 2 years ago

In this repo are some other examples of invoking a package manager – for instance install_gems will transparently take care of caching for us. That would add some benefit for us.

One other suggestion while I'm at it – in this case it seems we're invoking npm, so we should probably call that out. We use install_$foo for out command name, so it makes sense to have install_npm_packages – I could see us also having an install_yarn_packages command to invoke that package manager.

I've renamed the command to install_npm_packages and also added support for caching in 5a4e5b38611f185b91b2228e73ff5fd9b75325a3