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

Speed up on Node #16

Open
ai opened this issue Jun 14, 2019 · 5 comments

Comments

@ai
Copy link
Collaborator

commented Jun 14, 2019

Now we have 3 binaries in npm and script which run necessary binary depends on the current platform.

There are 2 big problems with current behavior:

  1. npm package size is huge (26 MB)
  2. Node.js startup time is not great.

Solution:

  1. postinstall script download necessary binary (many npm packages use this approach, for instance puppeter).
  2. npm runs binary directly without index.js
@ai

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 15, 2019

On CI we will avoid even downloading binary

@fleischie

This comment has been minimized.

Copy link

commented Aug 2, 2019

Hello all, especially @HellSquirrel (as you assigned this task to you),

How do I ask politely if I can offer my help with this task?

I made a prototype of how to download the binary on postinstall I made a crude mockup that downloads the appropriate binary for the current OS (or fail if the OS isn't supported), and afterwards run the `install` command with the downloaded binary. The postinstall script already had a check for CI in place, that can be used to skip the downloading in it's entirety.

I am unsure still how to handle errors on downloading the binary.

But I wanted to ask first, whether this task is still being developed on, and whether I should share my version with somebody.

Edit: Also I don't know where to store the binaries, if not in the .npm directory?
Edit2: I just went ahead and created a fork, so that I could share my work with you (as reference): fleischie@daa10c1
Let me know, if you need/want my help. 😄 👍
Edit3: I didn't actually test my branch as a postinstall hook (I just ran the script manually on Mac), so take with a grain of salt. 😅

@HellSquirrel

This comment has been minimized.

Copy link
Collaborator

commented Aug 2, 2019

Hi @fleischie ! Thank you for your question and interest in this feature.
I planned to do this task later, but if you want to help us it would be great!
If you already have a working draft just open a new pull request and we will continue the discussion there.

Also I don't know where to store the binaries, if not in the .npm directory?

I think you don't have to store binaries at all. We can load them from https://github.com/Arkweid/lefthook/releases

@ai

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 2, 2019

On network error we should try 3 times and print error message with 1 exit code if we still have network error.

Also we need environment variable to prevent downloading (for instance, if there too is not used on CI).

@ai

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 2, 2019

Puppeteer puts binaries to node_modules/puppeteer dir. We can do the same and put them in lefthook dir.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.