Skip to content

Conversation

@sirenkovladd
Copy link

Thanks for your pull request, we really appreciate contributions!

Please understand that it may take some time to be reviewed.

Also, make sure to follow the Contribution Guide.

Fix: go-task/go-npm#11

Change install from npm
postinstall script -> optionalDependencies

postinstall can be a vulnerable script, which is why some installations ignore all hooks
it also forces the installer to execute code making the installation slower and providing a bad experience

it is better to use optionalDependencies, which will allow you to install a binary image for a specific operating system

this will be faster and clearer, and will also prevent evil code to execute on your machine

@sirenkovladd
Copy link
Author

sirenkovladd commented May 29, 2024

not sure if I made the right GitHub action, better check it carefully

@Kenneth-Sills
Copy link

I agree that we should move to this approach. It also addresses a couple of issues:

  1. Right now, Task requires root permissions to install and always installs globally. If you install it local to a project, that project's dependencies will silently overwrite any existing Task installation. It will even cause conflicts with official package installations. You can always download the binaries, but it's an unnecessary barrier to entry that makes it harder to throw together a prototype, show coworkers, and push for adoption.
  2. The cleanup paradigm go-npm uses is out of date and since NPM 7 removed *uninstall lifecycle scripts, it leaves orphaned binaries lying around.

console.debug(`Creating folder ${folder}`)
await mkdir(`${folder}/bin`, { recursive: true })
console.debug(`Copying files to ${folder}`)
await writeFile(`${folder}/package.json`, packageJson.replace(/{platform}/g, platform.os).replace(/{arch}/g, platform.arch).replace(/{version}/g, mainPackage.version))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use .replaceAll('{platform}', ...)? don't this we need regex here.

Copy link
Author

Choose a reason for hiding this comment

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

What are the advantages of using replaceAll instead of replace with a global flag?

Copy link
Contributor

Choose a reason for hiding this comment

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

readers don't need to parse regex in mind

@caarlos0
Copy link
Contributor

caarlos0 commented Mar 9, 2025

fwiw, I'm implementing this in goreleaser as well... for what I can tell, this does not seem to work.

The platform-specific dependency is downloaded, but it seems the binary is never put in the right places.

npmx <the thing> also don't work 🤔

fwiw, here's the packages:

@vmaerten
Copy link
Member

Thanks for your PR
I am closing this because we use goreleaser now

@vmaerten vmaerten closed this Nov 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

install task binary with optionalDependencies instead of postinstall hook

5 participants