djipco / webmidi

Tame the Web MIDI API. Send and receive MIDI messages with ease. Control instruments with user-friendly functions (playNote, sendPitchBend, etc.). React to MIDI input with simple event listeners (noteon, pitchbend, controlchange, etc.).
Apache License 2.0
1.53k stars 115 forks source link

Make `dist` folder no longer pushed to GitHub #254

Closed Vexcited closed 2 years ago

Vexcited commented 2 years ago

Implemented proposal from https://github.com/djipco/webmidi/issues/253.

I tested the scripts by running them with --dry-run and seems like they worked. Well, I didn't tested them in a real environment where the modifications are really pushed and published.

The assets that will be published into GitHub Releases is the output (webmidi-*.tgz) of npm pack.

A .env.sample file was created which includes placeholders for the environment keys used in development. I have added GITHUB_TOKEN, so release-it can publish to GitHub Releases.

Before a release, npm library:build-all and npm pack are executed, you can see the full configuration of release-it in the package.json file.

djipco commented 2 years ago

Thank you so much for this PR. It looks good.

I merged it and ran it to check how it was performing. Overall it seems to work fine but there's a minor issue. The files that are published in /dist have comments inside with the library version. That version is behind because it is built before being bumped.

Also, I ran the script without the GITHUB_TOKEN. From the release-it documentation, I was expecting to be taken to the GitHub Release page to be able to do it manually but this didn't happen. The tag was created though.

Vexcited commented 2 years ago

The files that are published in /dist have comments inside with the library version. That version is behind because it is built before being bumped.

Oops, I forgot that !

If you want to bump and then pack what was published to NPM, all you need to do is move npm pack to the before:git:release hook.

"hooks": {
  "before:init": ["npm run library:build-all"],
  "before:git:release": ["npm pack"]
}

If you want to build right after the package version bump, instead of when starting the script, do this instead

"hooks": {
  "after:bump": ["npm run library:build-all"],
  "before:git:release": ["npm pack"]
}

Also, I ran the script without the GITHUB_TOKEN. From the release-it documentation, I was expecting to be taken to the GitHub Release page to be able to do it manually but this didn't happen. The tag was created though.

Well, this is unexpected... From their documentation, the manual mode is [...] is enabled automatically when the GITHUB_TOKEN environment variable is not set. You can still try to run the script with --github.web to set it to manual.

djipco commented 2 years ago

Well, this is unexpected... From their documentation, the manual mode is [...] is enabled automatically when the GITHUB_TOKEN environment variable is not set. You can still try to run the script with --github.web to set it to manual.

Yeah, I know. The weird part is that when I first ran it in dry-run mode, it did open a pre-filled web page for the release. I'm not sure what happened. I'm going to try with the token and see how it goes.

Regarding the version number, the second option should work. Trying it now...

djipco commented 2 years ago

We are almost there... I ran it once more and everything went smoothly. Thanks to your suggestion, the version was properly updated before building and the GitHub release was created automatically.

However, there's one last glitch. If you check out the release page, you will see that the tarball from the previous version is there. From what I can tell, the file created by npm pack is never disposed of.

Vexcited commented 2 years ago

However, there's one last glitch. If you check out the release page, you will see that the tarball from the previous version is there. I'm not sure why. Ideas?

Oh... I see what's happening here. When you run the script, it runs npm pack and this command results in the creation of a webmidi-*.tgz file in the project root. When uploading the assets to GitHub Releases, I used "assets": ["webmidi-*.tgz"]. So every existing pack are sent to GitHub Releases.

To prevent that, we can run rm webmidi-*.tgz before running npm pack. But it's not very stylish and should not work on every OS (I think).

I'll see if I find a better solution to this

djipco commented 2 years ago

To be on the safe side, I changed this:

"assets": [
  "webmidi-*.tgz"
]

... to this:

"assets": [
  "webmidi-${version}.tgz"
]
djipco commented 2 years ago

One deletion solution that works on all platforms is @alexbinary/rimraf. It is already a dependency.

Vexcited commented 2 years ago

"assets": [
  "webmidi-${version}.tgz"
]

Does it actually work ? I thought these variables were only available on hooks

Vexcited commented 2 years ago

I've found out that cat "$(npm pack)" gives us the .tgz file data so we can do cat "$(npm pack)" > webmidi.tgz and publish this webmidi.tgz asset, but I'm not sure if that works on Windows due to the cat command

Another solution would be to use a zip CLI package and create a zip file of the dist/ folder instead of npm pack.

djipco commented 2 years ago

Does it actually work ? I thought these variables were only available on hooks

I assumed it would. It is also used for commitMessage and releaseName. But now I'm not so sure anymore...

Vexcited commented 2 years ago

Well I think it would, now that you're saying it, it makes sense

djipco commented 2 years ago

It's hard to tell from a dry-run... I guess I will have to try it out...

Vexcited commented 2 years ago

I can tell it would work because of this line in release-it code. https://github.com/release-it/release-it/blob/master/lib/plugin/github/GitHub.js#L287

By checking the files, I guessed the format function is a function that converts these ${} variables (1st parameter) with the context (2nd parameter, the real values).

djipco commented 2 years ago

Given what you said, I decided to run it once more (after adding rimraf in the after:release hook). Everything worked perfectly. Yeah!

I really like the interactive aspect of release-it. It just feels like you actually know what's happening when you publish a new release.

Thank you so much for your help on this. 🙏🏻

Vexcited commented 2 years ago

No problem ! I really wanted to contribute to this project since it's really helping me in my projects. Well that's not a huge contribution but yea..

By the way, good thing you did to remove the output after the release, I didn't even thought about that 😭

djipco commented 2 years ago

Don't sell yourself short. This PR involved modifying many files and understanding how the release pipeline works. Most users could not have contributed it. This is very helpful to the community and will make it easier for others to also contribute.

djipco commented 2 years ago

This PR caused a regression issue with Node.js which I had not anticipated. The files /dist/esm/package.json and /dist/cjs/package.json are needed by Node.js to determine which type of module to properly load. Symptoms of this were reported in issue #269.

To correct this, I modified the /scripts/library/build.js file to always create the proper package.json files at /dist/esm/package.json and /dist/cjs/package.json. All these files do is identify to type of module that is in the matching directory.