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

Replace postinstall script for npm packages with libc field in package.json #628

Closed
meskill opened this issue Nov 13, 2023 · 2 comments · Fixed by #633
Closed

Replace postinstall script for npm packages with libc field in package.json #628

meskill opened this issue Nov 13, 2023 · 2 comments · Fixed by #633

Comments

@meskill
Copy link
Contributor

meskill commented Nov 13, 2023

Prerequisites

We have a scripts inside ./npm that generates npm packages published on npm that allow for easier installation of tailcall cli on different systems.

Describe the bug

  1. npm/post-install.js doesn't work as expected due to usage of the result of async call inside sync code. So, the final name of the package is wrong and no package is installed actually. The script came from Rewrite npm publish #607
  2. on linux despite system's libc both gnu and musl versions are installed (i.e. twice of required space is occupied) and I can't run "tc" command probably due to the naming clash. The script from above in theory should fix the problem on linux but it's actually does nothing. At least on linux, on other os I expect optionalDependencies should work

Expected behavior

The easiest way to fix this would be using libc field as it is should be supported by npm and other package managers. At least swc uses this approach

After that the postinstall script could be removed.

The main concern would be only compatibility with legacy versions of npm and other package managers

Testing published packages

We want to make sure that everything will work as expected and we won't face new problems with that. To do that we may want to implement tests for published packages in ci.

The implementation could look like this:

  1. for main target systems (windows, macos, unix-glibc, unix-musl) run test job
  2. use local registry as verdaccio to run local registry in ci
  3. publish npm packages in local registry (may require changing publish script to specify registry)
  4. create temporary dir
  5. install @tailcallhq/tailcall from running local registry
  6. make sure that commands tc --version works
  7. also show the output of npm ls -a to verify that only required packages are installed
@ologbonowiwi
Copy link
Contributor

@meskill can you review #633?

@meskill
Copy link
Contributor Author

meskill commented Nov 14, 2023

@meskill can you review #633?

Thank you, that looks good. I think now we need to verify that everything works on different platforms. I've updated the issue description with ci testing description. Would you mind giving a try to implement this as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants