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 prepublish script #6826

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

Conversation

signalwerk
Copy link
Contributor

This PR will allow people to install specific versions from GitHub (Git)

Why is this Pull Request needed?

If someone would like to use a specific Git-Branch via npm (npm install {{username}}/hls.js#feature/xy) in their project the build is missing and it won't work. since prepare was already used for the husky I took prepublish that is also part of the life cyles used to install packages (npm ci and npm i). With this you can use cusom hls.js branches in your project.

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

@robwalch robwalch requested a review from tjenkinson November 2, 2024 13:29
@tjenkinson
Copy link
Member

Hmm I think this could break ci when we publish as it will also then try and run the build again.

Given we publish canary releases to npm on commits to master not sure if we need to support installing via git?

@robwalch
Copy link
Collaborator

robwalch commented Nov 2, 2024

If someone would like to use a specific Git-Branch via npm (npm install {{username}}/hls.js#feature/xy) in their project

I don't think this is something we want. You could always pull down a fork or branch, build it, and include it in your project via npm link. The publish task is for publishing HLS.js releases.

The canary builds are listed in the deployments branch:
https://github.com/video-dev/hls.js/tree/deployments

@signalwerk
Copy link
Contributor Author

Oh. Sorry. I did not realise that the package.json in the NPM published package is also containing all the scripts. Would you accept if I add a build step, that is cleaning that for the publication? so that you get a clean package.json in the published package? something like for example parcel is having in their published package? I think for NPM there is no need to include the scripts and in that step we could also take out the devDependencies in the NPM package.json. That would also result in faster installs.

@robwalch
Copy link
Collaborator

Oh. Sorry. I did not realise that the package.json in the NPM published package is also containing all the scripts. Would you accept if I add a build step, that is cleaning that for the publication? so that you get a clean package.json in the published package? something like for example parcel is having in their published package? I think for NPM there is no need to include the scripts and in that step we could also take out the devDependencies in the NPM package.json. That would also result in faster installs.

See build.yaml and scripts/publish-npm.sh. We'll review any changes to this PR that update the publish config and scripts.

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.

3 participants