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

Remove releases/ folder #1230

Merged
merged 6 commits into from
Nov 28, 2021
Merged

Remove releases/ folder #1230

merged 6 commits into from
Nov 28, 2021

Conversation

ronyeh
Copy link
Collaborator

@ronyeh ronyeh commented Nov 14, 2021

Coming soon in a separate PR: a script based on npm ci that will download any old releases you specify, to support visual diffing between versions.

In a future PR: an interactive script automating the publishing of the NPM package, that is a bit less magical than grunt-brump and grunt-release.

@ronyeh ronyeh marked this pull request as draft November 14, 2021 20:55
Temporarily removing diff of current vs release upon git push, since we removed the releases/*.

We currently rely on npm run test:reference to do the visual diffing,
and will reintroduce automated visual diffing in a later PR
(e.g., via a script that downloads the previous release from NPM).

grunt-bump and grunt-release are a bit too automated / magical.
We can revisit npm release automation via an interactive script that
uses native npm commands, such as:
  npm version
  npm pack
  npm publish --dry-run
This automation script can also use native git commands like:
  git commit -m "VexFlow 4.0.0"
  git tag -a 5.0.0 -m "VexFlow 5.0.0-beta"
@ronyeh ronyeh marked this pull request as ready for review November 15, 2021 06:31
This was linked to issues Nov 16, 2021
@ronyeh
Copy link
Collaborator Author

ronyeh commented Nov 16, 2021

OK this one is ready to go.

It also fixes the grunt warnings, because it removes the grunt-release package which was last updated 5 years ago. (The bad dependency was shelljs.) We can automate our NPM release in a different way through an interactive node script (that invokes npm commands and package.json scripts).

@0xfe
Copy link
Owner

0xfe commented Nov 20, 2021

Conflict to fix.

So what do we lose with this? Is there any information loss at all from not checking in the releases? Are NPM + git tags sufficient? Are map files preserved?

@ronyeh
Copy link
Collaborator Author

ronyeh commented Nov 21, 2021

What information do we lose? That's a great question. To be extra safe, you could create a new repo called 0xfe/vexflow-releases/ which includes all the relevant release files for each version. The top level folders would be

vexflow-releases/4.0.0/
vexflow-releases/3.0.9/
...
vexflow-releases/1.2.93/
... etc ... 

But assuming that NPM doesn't disappear, we can always get the build files for any revision by doing "npm install [email protected]" This means old releases are also browsable via unpkg and jsdeliver.

Since I'm the one removing the releases/, I can commit to working on a script which pulls down the releases as necessary from npm (via npm install or npm ci).

Git tags are useful, assuming we never delete the tag, which would mean we lose the reference to the commit hash. But from version 4.0.0 onward, we no longer check in the build files, so the git tag will only take us back to the source files. We would still have to run the build again. So, if we want to be extra extra safe, we'd want to have a separate vexflow-releases repo. We can then install that repo as an optional submodule in vexflow, so that we could pull down old releases if necessary.

Regarding source maps: A comment by @sschmidTU #1231 (comment) suggested that maybe we don't need source maps on NPM. What do you think? :-) In my ESM branch I've temporarily disabled source map generation and 1) it makes webpack build much faster and 2) our build/ folder and npm package is super nice and clean looking. How often do you think people rely on source maps? If they need them, could they just build VexFlow from source and then turn on the sourcemap flag?

Anyways, let me know your opinion on source maps inside the NPM package. I think Simon said he just puts them in the OSMD GitHub release zip file, but does not put them on NPM.

@ronyeh
Copy link
Collaborator Author

ronyeh commented Nov 21, 2021

Tested this, with no visual diffs (via npm run test:reference). After this is merged, I'll try to finalize the proper ES module support so Simon can build OSMD with a light version of VexFlow that includes only Gonville. After the ESM PR is merged and tested, I'm happy to declare beta 2. I can work on a script to pull down old release builds from NPM while folks are testing out beta 2.

@0xfe
Copy link
Owner

0xfe commented Nov 21, 2021

Ron, instead of a new repo, how about Github releases? I've used that with other projects, and it's really easy to setup and automate.

Re: source maps, we need it somewhere, not necessarily in the NPM packages. We could build and bundle them with the github releases too. The criteria here is that we don't want people to have to build source maps for a version, they should be pre-built and downloadable for reach version.

We can definitely remove them from default builds -- they're only needed for versioned releases.

@ronyeh
Copy link
Collaborator Author

ronyeh commented Nov 21, 2021

Okay. GitHub releases sounds good. I haven't used it before, so will have to learn it.

@ronyeh
Copy link
Collaborator Author

ronyeh commented Nov 21, 2021

This seems like a decent replacement for grunt-release. It works with GitHub releases.

https://github.com/release-it/release-it

152,543 weekly downloads and last updated 2 days ago. Seems promising.

@0xfe
Copy link
Owner

0xfe commented Nov 28, 2021

Thanks Ron. Merging.

@0xfe 0xfe merged commit 7174bca into 0xfe:master Nov 28, 2021
@ronyeh ronyeh deleted the remove-releases branch January 20, 2022 00:14
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.

OffscreenCanvas types missing Warnings during grunt build
2 participants