search

pmotschmann / Evolve

An incremental game about evolving a civilization
Mozilla Public License 2.0
801 stars 344 forks source link

Automatically compile and update static assets #1055

Closed fredden closed 5 months ago

fredden commented 5 months ago

Ideally compiled assets aren't checked into the repository, and are only ever used / needed on the production instance / website. Meanwhile, I've seen at least a few instances where the compiled assets aren't updated when the corresponding source files are changed. This pull request automates this step. If the assets are already up-to-date, then nothing will happen. If the assets are outdated, this workflow will add a new commit to the branch updating the assets.

This doesn't solve the merge conflict issue with having compiled assets committed to the repository, but it does lighten the burden on new contributors of keeping the assets updated. It should also avoid the problem which we have in adcc3c2441ab0932af16e989053d246fd5cf6daa where a merge conflict was accidentally committed. Luckily the resultant file is still parsed and valid CSS is extracted, so the website isn't completely broken in this instance.

https://github.com/pmotschmann/Evolve/blob/fe8740e4c0f6d4a14c8563b52a7c7b2dddb600f7/evolve/evolve.css#L1

xilexio commented 5 months ago

I'm against this change because the root of the problem is that the generated files are in the repo in the first place. They aren't even useful there as a method of distributing Evolve to run locally by non-technical users, because index.html cannot be ran directly, as that'd require importing evolve/evolve.js and some CORS rules prevent that, at least in Google Chrome. They should either be just built and deployed by whoever downloads the code or available as downloadable packages. The solution in this PR is certainly better than the current situation, but it'd add clutter to the commits. And also won't fix any problems with conflicts due to accidentially commited generated files from multiple PRs.

Instead, I propose to just get rid of all generated files, make them go to an ignored dist directory and all static files should be copied there. Like that, it is also easier to make a GitHub pages deploy without junk files and the generated files won't even be present in git status, so they also won't be accidentially commited. I'd like to make a PR with that later on, after/if my PR #1061 with some preliminary cleanup is merged and hopefully I get any sort of feedback.

fredden commented 5 months ago

I agree that this is not the ideal end state. See the first sentence of the pull request description. I too would like to see these assets removed and the deployment process changed to include compilation of the same.

I didn't have time to do the whole GitHub pages reimplementation when writing this. Yes, that would replace this pull request when ready.

xilexio commented 5 months ago

I'm unsure what you mean by GitHub pages reimplementation, but in PR #1061 I just added an npm script that copies necessary files to a new dist directory and then publishes that on GitHub pages. Do you think that solves the problem in a better way? Either way, publishing would need to be done manually because the program should at least be ran to check if there aren't any immediate errors. It'd also be possible to use my deploy command to do npm run deploy in GitHub actions, which would enable testing (hopefully on a fork) and merging 100% on GitHub without downloading it locally.

If #1061 gets accepted, my next PR on that will be changing the scripts to put all generated files to dist, ignore them with .gitignore to finish the problem with leaving compiled files in the repo (which leads to conflicts like your conflicting #950, where you added the compiled main.js).

fredden commented 5 months ago

Superseded by https://github.com/pmotschmann/Evolve/pull/1064