Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change install from npm scripts postinstall to optionalDependencies #1675

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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

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
3 participants