Deploy master examples

[Galadirith]

Description

Add automated distribution building for master. This supports deploying the examples via GitHub pages. Because this PR is designed to operator on on the master branch it cannot be tested directly in the fork. However a demonstration branch Galadirith:deploy-master-examples-fork has been added with minimal changes to test and demonstrate this fork. You can view the minimal changes and the automated distribution build at Galadirith/aframe-physics-system@deploy-master-examples...Galadirith:deploy-master-examples-fork.

Once this PR is merged, simply go to https://github.com/n5ro/aframe-physics-system/settings, scroll down to the GitHub Pages section, select the master branch and / (root) folder, and finally press save.

Changes proposed

• Update dev command
  The dev command is now serving the built distribution from dist/aframe-physics-system.js. This makes it consistent with the updated examples so that now the examples can be used locally for development or hosted on GitHub pages without having to manually modify the src location of the script.

• Update src location in examples
  Again to make things consistent between local dev and GitHub pages hosted. In theory this could have been changed to any url as long as it was consistent, but as @MignonBelongie and others have suggested /dist/* is the standard location to commit built distributions, so it makes sense to use that url for both local and hosted files.

• Add builder workflow
  This new GitHub workflow will build the following files every time commits are pushed to master (either through a merge or direct push).

  /dist/aframe-physics-system.js
  /dist/aframe-physics-system.min.js
  

  This means developers do not have to manually run npm run dist and commit the updated /dist/* files, and I would recommend that developers do not manually build and commit their own distributions. Of course there will be instances where it is useful to manually build dist's, like on development branches. In general the idea is to minimise the number of update dist commits.

  To enable this, GitHub Actions will need to be enabled in this repo, as also mentioned in Add GitHub workflow to test Chrome and Firefox #168.

[Galadirith]

I have updated the workflow in a9b24cb to only build distributions when the actual source files change. This makes sense because we don't need or want attempts to build distributions if we are only changing auxiliary files like examples/ or **.md. This is equivalent to what is done in A-Frame core, just using different paths to reflect the different names in aframe-physics-system.

This change may actually mean we will need to perform a one-off manual npm run dist and commit on master, it depended on how GitHub performs the diff. But that's easy enough to check when this PR is accepted for merge.

[Galadirith]

Additional changes proposed

• Add example summaries and links
  A link to the a new examples examples/README.md has been add to the root README.md in 565969d. The new examples/README.md contains a summary of each of the examples, links to each of the examples, and where relevant links to the specific features of aframe-physics-system that the example demonstrates.

  While not a targeted feature of the PR, by deploying the examples to GitHub pages, by default all the *md files in the specified deploy folder (in this case / (root)) also get translated to html and deployed. For this PR a demonstration can be seen at https://galadirith.github.io/aframe-physics-system/. The new examples/README.md page can also be seen rendered at https://galadirith.github.io/aframe-physics-system/examples/.

  On the examples page you can click on each of the example links which will take you to the live deployed page for that example. It means that it's easy for people in VR to quickly navigate between different examples.

  One issue with the default GitHub pages deployment of examples/README.md is that none of relevant links to specific features of aframe-physics-system get correctly translated. Since the intention of this PR was to provide correct links when visiting the repository webpage at https://github.com/n5ro/aframe-physics-system/ and not to provide correct links for deployed rendered html, I do not consider this an issue. However it is something that can be revisited when a refactoring of the documentation is done after outstanding issues have been resolved.

[MignonBelongie]

I think this is great, but I have one concern: I think that it might be confusing to have a version of aframe-physics-system.js loaded in memory that's different from the one in the file system.

I experimented with a different version of dev:

"dev": "concurrently \"http-server -p 8000\" \"watchify index.js -o dist/aframe-physics-system.js -v\"",

The difference is that it saves aframe-physics-system.js every time there's a change.

You mentioned recommending that developers not commit files in dist, but unless it's enforced, we have to account for that possibility anyway. There are two possibilities if a commit does include dist: the automated build creates identical versions or different versions. In the latter case, the automated versions will replace the previous ones, which is good. In the former, I think the commit will fail because there will be no changes. I don't know the best way to deal with that, but I think we need to handle it, even with your solution.

Now, maybe there are other reasons not to save the built files, such as performance. I'm not convinced my suggestion is better, I just wanted to propose it.

[MignonBelongie]

the following collection of examples has been prepared.

[Galadirith]

Thanks for catching this 😊 I've fix this grammar in 79d3ada.

[Galadirith]

Thanks @MignonBelongie again I really appreciate the time you have taken to look through all my changes.

I think this is great, but I have one concern: I think that it might be confusing to have a version of aframe-physics-system.js loaded in memory that's different from the one in the file system.

Thanks so much for you're alternative proposal for the dev command. There were a few reasons I want to keep it with in-memory. The first is that's what the repo was already doing, so just making the minimal change possible. The second was practical, I'm still running with an older mechanical HDD so it always adds extra time whenever I develop on libraries that have to write to disk every time. APS is small enough that it probably isn't noticeable. The third is separation of concerns, the dev command isn't meant to actually produce builds, that's the point of the dist command.

But looking around some of the libraries that I use there is no one common choice. In A-Frame it is not saved to disk every time, however for Vue.js and three.js is saved every time. For those two repos it seems the only way they enforce preventing dist commits with every PR is with comments in their contibution guides (three.js, Vue.js).

I'am grateful for you bringing this point up. I do personally prefer it the current way, and I can't see a use case that would benefit from having the development build written to disk every time. However if there is a majority that would prefer writing to file every time, and there is a reason to do it, I'm happy to figure out an appropriate change based on your suggestion. Do you have a use case that would benefit from having it written to disk every time?

If this is changed, I would advocate for having two dev commands, one in-memory and one not.

You mentioned recommending that developers not commit files in dist, but unless it's enforced, we have to account for that possibility anyway.

Yes I again agree with you on this. We can make sure to clearly state not to commit the dist folder in a contribution guide, which we should add after getting through the outstanding issues. Some people will still add it, and I think we can deal with it on a case-by-case basis. I think this is an additional reason to not write out the dev build to file every time. If it's not there to be committed then someone will not accidentally commit it. Someone could still run npm run build, but with in-memory dev this at least reduces the likelihood of a modified dist files for someone to commit.

There are two possibilities if a commit does include dist: the automated build creates identical versions or different versions. In the latter case, the automated versions will replace the previous ones, which is good. In the former, I think the commit will fail because there will be no changes. I don't know the best way to deal with that, but I think we need to handle it, even with your solution.

Thanks for raising this point. Yeh I agree it will fail because there are no changes, in my test branch there is an example of this happening. I was happy with the outcome because, unlike the automated tests in #168 that will eventually hopefully be merged soon, if the dist fails it doesn't really matter. But it would still indicate on the commit status that it's failed, even though test's have passed, which wouldn't be desirable.

So we could add in a guard to the CI that checks to see if there are uncommitted changes. Based on an answer from VonC I think the following would work

git update-index --refresh 
git diff-index --quiet HEAD dist || git commit -m "Update built distributions"

I just need to check what shell the GitHub run commands are executed in. I think || is perfectly fine to just use, but maybe there is some exotic shell that is being used that prevents ||. This can easily be changed with defaults.run if necessary.

The git push command doesn't error if there is nothing to push, so we should only need to guard the git commit command.

[MignonBelongie]

Do you have a use case that would benefit from having it written to disk every time?

No, I don't have a use case. It's just that I can imagine myself, before I understood how budo worked, being utterly confused about why the code in the browser was different than in the file. I suppose it's something you just have to learn, and we can try to help in the documentation.

There are two possibilities if a commit does include dist: the automated build creates identical versions or different versions. In the latter case, the automated versions will replace the previous ones, which is good. In the former, I think the commit will fail because there will be no changes. I don't know the best way to deal with that, but I think we need to handle it, even with your solution.

Thanks for raising this point. Yeh I agree it will fail because there are no changes, in my test branch there is an example of this happening. I was happy with the outcome because, unlike the automated tests in #168 that will eventually hopefully be merged soon, if the dist fails it doesn't really matter. But it would still indicate on the commit status that it's failed, even though test's have passed, which wouldn't be desirable.

So we could add in a guard to the CI that checks to see if there are uncommitted changes. Based on an answer from VonC I think the following would work

git update-index --refresh 
git diff-index --quiet HEAD dist || git commit -m "Update built distributions"

I just need to check what shell the GitHub run commands are executed in. I think || is perfectly fine to just use, but maybe there is some exotic shell that is being used that prevents ||. This can easily be changed with defaults.run if necessary.

The git push command doesn't error if there is nothing to push, so we should only need to guard the git commit command.

If this works, it would be nice, but I think based on your example above, the behavior as-is is acceptable.

[Galadirith]

If this works, it would be nice, but I think based on your example above, the behavior as-is is acceptable.

Thanks @MignonBelongie 😊 I will go ahead a make that git change to build.yml. It was an important point you raised, and I do think it makes sense to have that conditional behaviour that will make the workflow better. I'll push that up sometime tomorrow, and then everything should be ready 👍

[Galadirith]

No, I don't have a use case. It's just that I can imagine myself, before I understood how budo worked, being utterly confused about why the code in the browser was different than in the file. I suppose it's something you just have to learn, and we can try to help in the documentation.

Yeh I think there is a lot we can do documentation wise to make this repo more inviting to new contributors, from code's of conduct to detailed contributors guide lines. I completely agree that budo and a lot of the other dev tools used for this repo can seem very confusing, and opaque. If someone didn't have experience with them before, it will be confusing and difficult. And I absolutely agree it's something we can help in the documentation.

[n5ro]

I closed this repo prematurely, just a mistake I made, sorry guys, I've opened it back up, and we will have a meeting about this and the other PR's soon!

[Galadirith]

Thanks again @MignonBelongie @Micah1 for taking a look at this PR. I've made the final modification in
99c3940 that conditionally makes a git commit only if there are actually changes to the dist folder. You can see a demonstration of this on my fork Galadirith/aframe-physics-system@deploy-master-examples...Galadirith:deploy-master-examples-conditional. You can see that 5185f48 triggers a build, and then the workflow commits the build in ba233b2. I then trigger a build again in 4715457 knowing that the distribution will not change, and as expected there is no additional commit from the workflow. Importantly now the workflow for 4715457 no longer fails.

If you take a deeper look at the workflow logs, for the first triggered workflow you can see the commit of the built distribution. For the second triggered workflow you can see that there is no attempt to commit due to the extra condition introduced in 99c3940 as desired.

I have no more changes I would like to make, so this is ready to merge from my side. Please let me know if you have any other questions.

[MignonBelongie]

One issue with the default GitHub pages deployment of examples/README.md is that none of relevant links to specific features of aframe-physics-system get correctly translated. Since the intention of this PR was to provide correct links when visiting the repository webpage at https://github.com/n5ro/aframe-physics-system/ and not to provide correct links for deployed rendered html, I do not consider this an issue. However it is something that can be revisited when a refactoring of the documentation is done after outstanding issues have been resolved.

I understand that it's not in scope to fix the broken links, but wouldn't it be better to leave them out until they work? In any case, we should add another issue, to either add or fix these links.

[Galadirith]

Thanks so much @MignonBelongie yeh I agree with your suggestion, it makes sense to not include those extra links that are not critical, untill there's a solution that makes them valid both through GitHubs repo interface and deployed through GitHub pages. I'll remove those links and push that shortly 😊

All the example links will still work, with them linking to live deployed examples when viewing the documentation deployed through GitHub pages, or linking to the HTML source when viewing in GitHubs repo interface. And I think that makes sense, as you probably want to see the live example when on GitHub pages, but you want to see the source when in the repo interface. But please let me know if you disagree.

[MignonBelongie]

@Galadirith Sorry, I forgot about this. Do you have time to remove the links? If not, we might merge this tomorrow anyway and just create another issue to fix them.

As for your question, I think having the links go to the source when you're in the GitHub repo is good. I'm actually trying to find a good way to also have links to the examples when running locally (i.e. after running npm run dev then going to localhost:8000), but that's for another PR.

[Galadirith]

@MignonBelongie I really apologise for not having been able to reply sooner. Thanks so much for merging this in, I didn't have time in the end to remove the partially broken links. However I agree, I think it's useful to have them there when browsing the GitHub repo, and they can be updated to be applicable to both the GitHub repo, GitHub pages, and local render in another PR. Thanks so much again 😊

Really awesome to now see live examples at https://n5ro.github.io/aframe-physics-system/examples/ that will keep in sync with the automatic master dist build as features get added and bugs fixed 😊