Keeping forks up to date

[gfwilliams]

There are a bunch of apps that end up being copies of other apps/code but with some changes applied.

Often that original code will have some changes/bugfixes later down the line and it's always a pain to ensure that those get propagated.

Discussed:

• https://github.com/espruino/BangleApps/commit/625f31c8d44258bd977ce1a757a720e7da5f8ad0
• https://github.com/espruino/BangleApps/pull/2843#issuecomment-1612147866

The apps might be copies of other apps in the repo, or of built-in functionality that is in other places like https://github.com/espruino/Espruino/blob/master/libs/js/banglejs/E_showMenu_Q3.js

So how do we keep these up to date?

As a simple, fast solution, I'm coming down on something like an entry in metadata.json:

{
  // ...
  patches : [
   { ours: "app.js", theirs : "https://raw.githubusercontent.com/espruino/Espruino/master/libs/js/banglejs/E_showMenu_Q3.js", patch: "appchanges.patch" }  
  ]
}

So then (maybe even just in sanitycheck.js) we just grab the file from the URL given, attempt to patch it and compare the result, then flag up a warning if the two differ or the patch fails to apply?

[bobrippling]

Yeah I like the sound of that, since our changes in runplus are pretty slim or I imagine similarly with swscroll.

With theirs: "app/run/app.js", I don't see a problem, since we can only break sanitycheck.js in the commit that changes run/app.js. Whereas if we have theirs: "https://...", then the build could break at an arbitrary time. So perhaps we make the former hard errors, and the latter warnings?

[thyttan]

I'm in favor generally. I don't have too much input regarding technicalities - sounds like a good solution @gfwilliams !

How would a patch file look?

[nxdefiant]

I think the list of possible URLs should be whitelistet to avoid a repository not under control to inject malicious code.

[gfwilliams]

Ok, sounds good - non-http links where possible is a much better idea. And as you say, only warn for stuff that's not in the repo.

whitelist

I think for now we only make it load from https://raw.githubusercontent.com/espruino/... URLs - or maybe even don't do HTTP at all and only support app/... and Espruino/...

[bobrippling]

There's also another approach, which is a bit more dev-heavy. We might be able to modify the upstream in some cases (e.g. swscroll) to accommodate downstream changes / extensions to their behaviour.

I've done something like this with sched in #2781 to allow multitimer to drop its copy of sched.js and plugin to sched instead.

Of course this only works with functionality, rather than whole apps like runplus.

[gfwilliams]

We might be able to modify the upstream in some cases (e.g. swscroll) to accommodate downstream changes

... yes, in some cases it makes sense. But also I've seen over and over this issue where we just add more and more and more stuff in to apps that were simple, and they just get bigger, and slower, and harder to maintain.

It's not immediately a massive issue on Bangle.js 2, but on Bangle.js 1 (which people still use) where RAM is more limited it was actively breaking the watch. So in many cases if someone wants to put a bunch of extra functionality into an app, it really is better to have a new app for it and have the original, and a ++ version.

Anton clock was a good example - it was designed to be simple with easy to read code, and to ship with the watch out of the box. But then a bunch of features got added and the end result was that the default app that shipped with the watch looked identical to before, but took twice as long to load, used more battery, and was pretty daunting for anyone looking to use it as a starting off point for Bangle.js development. In the end I reverted the changes and put them in a new antonplus app.

[bobrippling]

Ah yeah, I hadn't considered Bangle.js 1, that's a fair point. In that case I'll vote for the original patch idea too - thanks for the surrounding detail!

[nxdefiant]

I have another use case: Patch openstmap for Bangle JS2 to display a marker with an overlay at the current location.

(This comment is mainly for me personally to pick up on this idea when this feature is implemented)

[gfwilliams]

Patch openstmap for Bangle JS2 to display a marker with an overlay at the current location.

That sounds like a nice idea - why don't you just implement it in the main app? I think that would benefit everyone (although you'd have to do it only on Bangle.js 2).

[nxdefiant]

I thought I avoid the dead code for Bjs1, but ok might do it in the next days.

[pavelmachek]

I guess traditional solution would be discouraging forks. If the code is shared between two applications, it likely belongs to library...

[bobrippling]

We've got some changes/a fork to the rebble app (#3137), we could attempt to merge the changes into the original rebble clock and put them under an option, any thoughts?

I'd vote to merge it, we can reconcile the changes without too much difficulty still

[thyttan]

I feel this from the wiki indicates adding it as settings in the original rebble does make sense:

If the original app is bigger (especially if it already has a settings page), could the change be implemented as a setting? Or if it's just a good feature/bugfix maybe it should just be added by default to the original app.

Maybe we ask if they could preferably implement it in rebble as settings, but offer to merge the fork instead as well? I could help with answering questions/giving review suggestions re pulling it into rebble.

[Hairo]

Oh sorry, didn't see that page, might be a good idea to mention it in the add app page, i'll try to make the changes an option in rebble soon, will submit a new PR when it's ready.

[thyttan]

might be a good idea to mention it in the add app page

Thanks for the suggestion! I'll look at that :)