documentcloud / underscore-contrib

The brass buckles on Underscore's utility belt
MIT License
621 stars 117 forks source link

Take generated files out of version control #228

Closed jgonggrijp closed 3 years ago

jgonggrijp commented 4 years ago

I agree with https://github.com/documentcloud/underscore-contrib/issues/211#issuecomment-679136651 by @joshuacc. Same applies to the dist files. We can tell git to delete them, add them to the .gitignore, and then continue business as usual.

jgonggrijp commented 4 years ago

@joshuacc I was about to do this, mostly because I'm getting annoyed at having to uncheck the modified dist files every time I commit through my editor. However, there is a catch.

If we add the generated files to the .gitignore and remove them from VCS, we have to be very careful to undo these changes (i.e., remove the .gitignore lines and re-commit the generated files) the first time we merge master into gh-pages. At least, I hope we're going to adopt such a procedure for subsequent releases (see #172).

There is also the disadvantage that if people download a zip of the source code, they will get no documentation until they install the dependencies and run the build steps. There is something to say for having everything committed like in Underscore, too. In Underscore, there is even a pre-commit hook that keeps the non-minified dist files in sync with the source, so you can always download a bleeding-edge version from the website.

So, where do we take this?

joshuacc commented 4 years ago

I see a couple of different concerns here.

First, the generated JS files. Those can be generated as part of a pre-publish script instead of a pre-commit. Then there is no need to keep them in git at all.

Second, the generated HTML docs. I don't think those need to be present in git or the published package. The markdown docs are already available there. But if you do want them published in the npm package, just do it as part of the pre-publish script.

jgonggrijp commented 4 years ago

While I don't necessarily disagree, I think it is a bit more nuanced than you paint it:

I'm going to take a dive into GH Actions in order to enable CI. While I'm at it, I'll investigate whether it might could help us with this issue as well. It's easier to make choices when all the options are on the table.

joshuacc commented 4 years ago

I'm honestly fine with you doing this however you want, so take the following as you will. :-)

For the generated JS files, I don't see any benefit to keeping them in git. Bower is deprecated. If we want to offer a non-npm method of downloading the minifified version, that could just be hosted on GitHub pages as part of the docs site.

For GitHub pages generation, this action should help: https://github.com/marketplace/actions/deploy-to-github-pages

jgonggrijp commented 4 years ago

Thanks for the trust, and for the helpful pointer. :-)

I've exercised with CI (#230) and I plan to add CD in one go. Despite your kind gesture, I'm still going to ask your opinion, because I prefer to set up a scheme that both of us really like if I can help it.

The plan I'm going to describe may appear a bit complicated, but hopefully you'll agree it all makes sense. My main objectives are the following:

As I currently envision it, cutting a new release would consist of the following steps.

  1. Maintainer creates a release/x.y.z branch based on master.
  2. Maintainer bumps the version in the package.json to x.y.z, without creating a release tag. So yarn version --no-git-tag-version with the --major, --minor or --patch flag.
  3. Maintainer updates the change log and the documentation (markdown source) as necessary.
  4. Maintainer checks that tests, docs and annotated source (which I intend to add in the future) all look good.
  5. Maintainer submits PR for the release/x.y.z branch. At the very least, in order to trigger CI and to leave a trace, possibly also so that another maintainer can verify the change log.
  6. Maintainer merges the release/x.y.z into two branches: master and prepublish. prepublish will be a persistent branch, intermediate between master and gh-pages.
  7. CD workflow checks out the prepublish branch and attempts to execute the following steps in order, bailing out on the first error. a. yarn grunt dist docco tocdoc. b. Commit the build output. c. yarn publish (or npm publish if this works better with GH Actions). d. Tag the tip of prepublish as version x.y.z. e. Merge prepublish into gh-pages.
  8. If step 7 failed, maintainer adds fixes to release/x.y.z and tries again from step 6.
  9. Maintainer deletes the release/x.y.z. branch. The next merge into prepublish will have a version number greater than x.y.z.

To make this work, some one-time preparations would be required:

Please let me know what you think. If you have any hesitation, even a very slight one, I want to know it. I will not proceed until I have your feedback.

joshuacc commented 4 years ago

It looks a little more complicated than I prefer, but I have no fundamental objections. My only request is that the process gets documented in a markdown checklist somewhere so that maintainers can follow through on each item without losing where they were.

jgonggrijp commented 4 years ago

This exact process is not sacred to me, as long as we meet the goals. So if you have any ideas, even if they don't entirely meet the goals, I'd be interested to hear them. I'd prefer simpler, too, but haven't been able to come up with a simpler flow yet.

joshuacc commented 4 years ago

I suggest going with this. After it has actually been used a time or two, I think any important improvements will become obvious.

jgonggrijp commented 4 years ago

@joshuacc FYI, I have executed the branch changes as described above. We now have:

If you fetch and track these branches locally, you can run git flow init with prepublish as the production branch, master as the "next release" branch and v as the version tag prefix. No hurry though, this can wait.

I'll continue with CD in #230 soon.

jgonggrijp commented 3 years ago

I'm still tinkering with continuous deployment (see #238), but the files mentioned in the ticket title have been taken out of VCS in #230. Closing this.