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

do not commit generated files to VCS #2615

Closed
boneskull opened this issue Dec 2, 2016 · 10 comments
Closed

do not commit generated files to VCS #2615

boneskull opened this issue Dec 2, 2016 · 10 comments
Labels
area: browser browser-specific semver-major implementation requires increase of "major" version number; "breaking changes" type: chore generally involving deps, tooling, configuration, etc.
Milestone

Comments

@boneskull
Copy link
Contributor

When we added Bower support long ago (#584), it was implemented by committing the bundle (the root mocha.js) to VCS.

This, of course, is bad practice.

I have no numbers anywhere on how many people install Bower via Mocha, but it's probably enough to cause pain if we stop supporting it.

Instead, we can create a script that dumps the dist file(s) into another repository, a la AngularJS. The dist file(s) would still be published to npm.

Practically speaking, whenever anybody runs the browser-based tests via Karma, we generate a new bundle so that karma-mocha uses Mocha to test itself. That's inconvenient for contributors and causes confusion.


An alternative or supporting solution is to consume unpkg in some way. I'm a little hesitant to depend on this service. It's open-source, so we could run our own server, but $.

@boneskull boneskull added area: browser browser-specific type: chore generally involving deps, tooling, configuration, etc. labels Dec 2, 2016
@ScottFreeCode
Copy link
Contributor

Practically speaking, whenever anybody runs the browser-based tests via Karma, we generate a new bundle so that karma-mocha uses Mocha to test itself. That's inconvenient for contributors and causes confusion.

Isn't this necessary in any case so that when Mocha is self-testing it's testing the latest Mocha code?

@ScottFreeCode
Copy link
Contributor

We've had multiple PRs recently that only updated the generated bundle.

I seem to recall there was an issue or PR where we had more details on what it will take to make the change happen, but I'm not finding a more recent one than this; anybody remember which issue or PR it was?

@dasilvacontin
Copy link
Contributor

@ScottFreeCode idk. The only solution I have in mind right now is a script in CI that checks whether mocha.js has not been modified in the PR.

@ScottFreeCode
Copy link
Contributor

I think #2267 may have been the previous discussion I was thinking of, but it turns out there isn't much there that isn't already here.

@boneskull boneskull added the semver-major implementation requires increase of "major" version number; "breaking changes" label Sep 10, 2017
@boneskull
Copy link
Contributor Author

let's not go the angular route. npm is fine; unpkg is fine; bower users should stop using bower.

@dasilvacontin
Copy link
Contributor

I remember that @Munter mentioned about making "test" or whatever build "mocha.temp.js" instead of "mocha.js". He highlighted that one of the problems is that we overwrite the "build file" for testing, when the "build file" should only be modified when we actually want to "rebuild the published binary".

@ScottFreeCode
Copy link
Contributor

Yeah, we've got BUILDTMP/mocha.js now to fix that -- I think he set it up if I recall correctly!

@Munter
Copy link
Contributor

Munter commented Sep 11, 2017

Yeah. We should be in a place where contributors are able to make local changes and run the tests without creating publication build artefacts. Of course this is no guarantee that they won't run the build step to create those artefacts manually and check in the changes. We should still have a bot to highlight that error and block merging. But at least that case shouldn't happen a lot in recent times

@boneskull
Copy link
Contributor Author

boneskull commented Oct 1, 2017

OK, so this is what's gonna happen:

  • the preversion script will change to run test and maybe a dont-break check or two (which I'll poke at a bit later)
  • the prepublish script will change to run make mocha.js, which will cause the file to be published.
  • get rid of BUILDTMP and all references to it
  • mocha.js will be removed from VCS
  • mocha.js will be added to .gitignore
  • bower.json will be removed from VCS
  • bower.json will be removed from files prop of package.json

UPDATE Actually, if we remove mocha.js from VCS right now, it will only affect those Bower users who are using "mocha": "*". I'll just go ahead and clean it up, then start on the changelog.

@boneskull
Copy link
Contributor Author

this is done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: browser browser-specific semver-major implementation requires increase of "major" version number; "breaking changes" type: chore generally involving deps, tooling, configuration, etc.
Projects
None yet
Development

No branches or pull requests

4 participants