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

Let apps.json be generated by jekyll #1221

Merged
merged 15 commits into from Jan 19, 2022

Conversation

adamschmalhofer
Copy link
Contributor

@adamschmalhofer adamschmalhofer commented Jan 5, 2022

Reading

#1020
#46

My understanding is that the requirement for a split apps.json solution
is:

  • work seamlessly with github pages (no manually calling a script)

  • single (or at most a few) apps.json for the browser to load for the
    store so the store page is fast

As the github pages already use jekyll to build the store page this can
be solved by a minimal template. This is a proof of concept of
that. If the concept is accepted: in a next step, I will create the
split files for more that just the two here. In addition we will need to
update the documentation

Reading

espruino#1020
espruino#46

My understanding is that the requirement for a splitt apps.json solution
is:

  - work seemlessly with github pages (no manually calling a script)

  - single (or at most a few) apps.json for the browser to load for the
    store so the store page is fast

As the github pages already use jekyll to build the store page this can
be solved by a minimal jekyll plugin. This is a proof of concept of
that. If the concept is accepted in a next step, I will create the
splitt files for mmore that just the two here.
Instead of plugin as tried first.

Turns out github doesn't allow own plugins (unlike gitlab). It is the
simpler and more elegant solution any how.
remove unneeded layout definition in apps.json
@rigrig
Copy link
Contributor

@rigrig rigrig commented Jan 6, 2022

I'd also like a javascript merge script for local development/testing using npm start.
But I think that should be relatively easy using npm watch.

@adamschmalhofer
Copy link
Contributor Author

@adamschmalhofer adamschmalhofer commented Jan 6, 2022

I'd also like a javascript merge script for local development/testing using npm start. But I think that should be relatively easy using npm watch.

Or you could use jekyll serve

@rigrig
Copy link
Contributor

@rigrig rigrig commented Jan 6, 2022

Or you could use jekyll serve

Oh, cool. I didn't know jekyll could run standalone, figured it was just some preprocessor language for GitHub pages.
...too bad it doesn't just work :-(

$ jekyll serve
Configuration file: /home/richard/src/BangleApps/_config.yml
jekyll 3.8.6 | Error:  The jekyll-theme-minimal theme could not be found.
...

So it looks like it requires you to also figure out/setup jekyll/ruby, instead of just sticking with npm/javascript which we already use for everything else.

But I guess this works fine for people using GitHub pages, and if we go this way I'd be happy to look into making it work locally with just npm.

Some bikeshedding: I would prefer the per-app files being named app.json, just makes more sense to me to combine several app.jsons into one big apps.json.

as the folder starts with "_" jekyll ignores that. So no additional
logic needed in apps.json
@adamschmalhofer adamschmalhofer changed the title WIP Let apps.json be generated by jekyll Let apps.json be generated by jekyll Jan 7, 2022
@gfwilliams
Copy link
Member

@gfwilliams gfwilliams commented Jan 7, 2022

Thanks! So actually everything can be handled with the {%- assign apps = site.static_files | where: "name", "metadata.json" -%} ... code in apps.json?

I'll have to think about this as it's quite a big change, but it definitely seems to tidy things up a lot.

I'm in two minds about metadata.json - while I like app.json it's a bit confusing for widgets/etc, so probably keeping with what's here is better

@BartS23
Copy link
Contributor

@BartS23 BartS23 commented Jan 7, 2022

I like @adamschmalhofer's solution very much.

I had another idea:

It would also be solvable via an additional branch ( e.g. gh-pages).
With every commit the apps.json is rebuilt via actions and merged with the commit into the branch gh-pages.

You can find an example here: https://github.com/BartS23/BangleApps/blob/master/.github/workflows/PreparePages.yml

Both solutions also have the possibility to leave the existing apps.json unchanged and only use the app.json/metadata.json for new apps.

@gfwilliams
Copy link
Member

@gfwilliams gfwilliams commented Jan 18, 2022

Thanks for keeping this up to date!

I just posted a question about this on the forum to see what those outside of GitHub think: http://forum.espruino.com/conversations/372018/

@gfwilliams
Copy link
Member

@gfwilliams gfwilliams commented Jan 19, 2022

Ok, so everyone seems to think this is a great idea - let's merge!

I assume you have some kind of script/tool to update the metadata based on the latest apps.json changes? Please could you run it one more time and then I'll merge this in :)

@adamschmalhofer
Copy link
Contributor Author

@adamschmalhofer adamschmalhofer commented Jan 19, 2022

Updated. Looking forward to it being merged \o/.

@gfwilliams
Copy link
Member

@gfwilliams gfwilliams commented Jan 19, 2022

Excellent - thanks!

@gfwilliams gfwilliams merged commit 5e5d23c into espruino:master Jan 19, 2022
@gfwilliams
Copy link
Member

@gfwilliams gfwilliams commented Jan 19, 2022

@adamschmalhofer is it possible that when you ran your script, you pulled apps.json from banglejs.com/apps (which would have been out of date)? I'm seeing a few cases where apps didn't have metadata.json files or they are out of date

@gfwilliams
Copy link
Member

@gfwilliams gfwilliams commented Jan 19, 2022

Actually there was some odd stuff that had gone on - not sure how, but I think it's all fixed now!

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

Successfully merging this pull request may close these issues.

None yet

4 participants