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

Add support for all packages manager and shrink package size #1

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

marvinroger
Copy link

This PR is part of a fix for the hookdeck/hookdeck-cli#44 issue

I basically simplified the code to make it smaller and compatible with all packages managers:

  • I bumped the dependencies and adapted the code (there were deprecation warnings)
  • I removed the not needed esbuild build step which resulted in a whooping 1.62MB file! The src/index.js file is already valid JavaScript / Node.js code, so it cas be used as is, which drastically reduces the library footprint
  • The binary is not moved to the npm bin output anymore, as this is not needed and was npm-only. It only requires a bin field in the package.json, but I'll do a follow-up PR on https://github.com/hookdeck/hookdeck-cli
  • Therefore, no need for an uninstall command anymore. The binary is part of the node_modules and is not moved in a global solution, so there's no point in cleaning

@@ -5,10 +5,8 @@
const request = require("request"),
path = require("path"),
tar = require("tar"),
zlib = require("zlib"),
Copy link
Author

Choose a reason for hiding this comment

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

Newer version of tar already uses zlib under the hood if the file is a gzipped one

@@ -26,47 +24,13 @@ const PLATFORM_MAPPING = {
freebsd: "freebsd",
};

function getInstallationPath(callback) {
// `npm bin` will output the path where binary files should be installed
exec("npm bin", function (err, stdout, stderr) {
Copy link
Author

Choose a reason for hiding this comment

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

As stated in the description, this is only npm compatible, it was not compatible with Yarn and pnmp (same thing for npm_config_prefix env var).

Here is the magic: You ask to run `go-npm install` after it completes installing your package. This will pull down binaries from Github or Amazon S3 and install in NPM's `bin` directory. Binaries under bin directory are immediately available for use in your Terminal.

Edit `package.json` file and add the following:
```
{
"postinstall": "go-npm install",
"preuninstall": "go-npm uninstall",
"preinstall": "go-npm install"
Copy link
Author

Choose a reason for hiding this comment

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

The binaries are linked by npm / yarn etc. on install, so the binary has to be present before the installation. preinstall achieves it, whereas postinstall is too late

@alexbouchardd alexbouchardd self-requested a review April 11, 2023 21:43
Copy link

@alexbouchardd alexbouchardd left a comment

Choose a reason for hiding this comment

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

Fantastic work @marvinroger. I was in a rush when trying to deploy the CLI to NPM for our console.hookdeck.com release. Very thankful for your time 🙏

@alexbouchardd alexbouchardd merged commit 84d4c79 into hookdeck:master Apr 11, 2023
@alexbouchardd
Copy link

1.0.6 has been published to NPM

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.

2 participants