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 compiled files to repo #2206

Closed
jamesplease opened this issue Jan 26, 2015 · 9 comments
Closed

Add compiled files to repo #2206

jamesplease opened this issue Jan 26, 2015 · 9 comments

Comments

@jamesplease
Copy link
Contributor

I understand that there have been several issues about this topic, and I'm sorry to raise the issue once more! But I still can't figure out why the built files are in the repo, so if you're not completely sick of talking about it, I'd appreciate going over it once more. 🐱

I've seen two arguments:

  1. Bower does not support a postinstall step, or a system to manage built assets

This is true, but I don't see why this matters. Many other popular projects just include the built files in the repo. No big deal, right?

  1. It's a lot of work maintaining it

I admit I don't know much about the chosen release process, but I'd expect it to be 1 command that just generates the built files. You could then merge those builds with the same PR that updates the bower.json/package.json versions. The files are built either way, I expect, 'cause you include the zips in the release.

If you're uninterested in doing this out of preference, then that's fine. I'm just curious to know if there's some technical reasoning that's preventing it.

Oh, and I've seen the instructions in the repo for the zip file. I just expect that people are far more likely to go straight to bower install chosen, given that's how most other libraries work.

@jamesplease
Copy link
Contributor Author

This would also work to solve #1433.

@tjschuck
Copy link
Member

Not gonna happen. @stof said it best here.

Additionally, we do provide the compiled files. Pre-compiled versions are provided via the releases page. If your package manager of choice supports specifying packages via ZIPs, you should use them. See the bower instructions for an example.

If your package manager of choice does not support ZIPs or using the GitHub releases API, you should open an issue with them to add support for this.

@jamesplease
Copy link
Contributor Author

@tjschuck, thanks for the link to @stof's comments! I hadn't read that before, and it makes things much more clear.

I wonder if a few small changes to your process could resolve the problems you had. I manage and work on a handful of projects that publish the built files to the repo. If we had a similar system to the one described in that comment, I can definitely see how there would be problems.

I think two changes can fix the issue:

  1. Make the default grunt task only lint and test the files, but not build the final versions
  2. Only build the files when it's release time

This is how the other projects I work on operate, and it works great. Going through each of the problems in that comment...

  • nothing ensures that contributors would rebuild it properly each time (meaning that they could be out of sync)

Contributors would never be responsible for building the library, so this is a non-problem.

approximately 75% of contributions were changing the compiled files rather than the source (even though the contributing documentation was explicitly warning against this)

Marionette used to have the same problem. I switched the default grunt task to no longer build the files (it only runs the test), and we haven't had a PR build the library in over a year. In every other OS project I work on, I've done the same thing, and I can't remember the last time I've have an accidental build.

for PRs doing things fully, it would forbid having 2 concurrent mergeable PRs because of conflicts in the minified files

If all of the above is true, then this would automatically be solved for, too.

PRs were often sending lots of unrelated changes in the compiled code (thus increasing the potential for conflicts in it) because of using a different version of the compiler.

This should also be solved.

@tjschuck, I really appreciate the work y'all have done to make this project so awesome. But I think a few changes to the build process / release system could take it to the next level and make it a true slam dunk.

I also respect that your team has a process that's been working well, and I don't mean to come in here and tell you to switch it up. But if you'd be interested in revisiting this, I'm confident we could figure something out that works.

Thanks again for taking the time to review this issue even though I'm sure you're sick to death of talking about it!

@stof
Copy link
Collaborator

stof commented Jan 30, 2015

@jmeas the issue with your proposal is that compiled files in the repo would not contain the same code than the Coffee source, except for commits where the build was run. This makes providing compiled files irrelevant if we provide the wrong ones.

@jamesplease
Copy link
Contributor Author

@jmeas the issue with your proposal is that compiled files in the repo would not contain the same code than the Coffee source, except for commits where the build was run.

This is true, but you might be surprised to find that it is of little consequence to developers.

This makes providing compiled files irrelevant if we provide the wrong ones.

See, I don't think this is a problem. The fact is that nobody goes to Github to download compiled files from the master branch. For Chosen, this is trivially true today because the files aren't there, but even if you added them, it wouldn't happen. Nearly all developers get source code from either:

  1. Downloading the zip file from a release, or from the webpage, or CDN. In this situation, the built file is associated with a particular commit, and a particular Github tag.
  2. Using a package manager tool like Bower or npm. Bower, like your release zips, uses Github tags. npm uses the state of your repository at the time that you publish the release. As part of the release cycle, the tag is created at the same time that you publish, so they stay in sync without much effort.

It's interesting, but those out-of-date files do go mostly ignored. A whole bunch of projects do this, including Backbone, Marionette, Underscore, Lo-dash, Handlebars, and others. It's a consequence of wanting to use 1 repo for distributing your project on all package managers & using the master branch for development.

The counter-argument here, of course, is that you're suggesting that developers use tagged git releases to download Chosen. I worry that this method prevents developers from getting patch releases more easily, which is one of the purposes of using package managers. Instead, it requires that they manually update the zip URL whenever they want to increment the version, rather than just using the CLI to bump the version.

Either way, I guess it's not too big a deal; I just don't understand the justification for not including them.

@stof
Copy link
Collaborator

stof commented Jan 30, 2015

@jmeas another point is that half of the contributions we got before removing the compiled files were editing the compiled files instead of the source

@jamesplease
Copy link
Contributor Author

Right, right. I replied to that concern up here. The tl;dr is that if you can avoid it by:

  1. Changing the default grunt task to no longer build the library
  2. Ensure that it isn't necessary to build the library to contribute. In other words, linting and running the tests should work w.o. building the distribution files.

@stof
Copy link
Collaborator

stof commented Jan 30, 2015

Changing the default grunt task to no longer build the library
Ensure that it isn't necessary to build the library to contribute. In other words, linting and running the tests should work w.o. building the distribution files.

This does not solve it. the issue is that people are using the JS files in their own project, and so they go editing them directly. And having contribution guidelines about this does not help. At least half of the contributors don't read them until we tell them that their PR is done the wrong way.

@pfiller
Copy link
Contributor

pfiller commented Mar 28, 2015

bower install chosen

As of 1.4.2, Chosen supports bower installs. Read the 1.4.2 release notes for details.

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

No branches or pull requests

4 participants