espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.76k stars 743 forks source link

typescript: permit `Bangle.setUI({ ... })` with no `mode` #2389

Closed bobrippling closed 1 year ago

bobrippling commented 1 year ago

A small typing tweak from espruino/BangleApps#2871

gfwilliams commented 1 year ago

Do you mean literally just Bangle.setUI({})? I'm not sure we should allow that.

Normally, you should do Bangle.setUI() to completely clear everything out, or Bangle.setUI({mode:"...", ...}) - but I think if you're supplying an object you should supply a mode.

bobrippling commented 1 year ago

Ah sorry - I think the title may have confused the situation. What I should've said was that we want to allow mode to be omitted, while other entries can be passed in, like in messagelist's use (I suppose this is effectively a setUI() no-argument call, just that it's in the same codepath as a call that might include mode too).

My intention here is to allow development of wrappers around setUI in typescript, where typescript will guide a developer into handling the case where mode is missing in the argument object. Or even better, if we do manage to get typescript to check the javascript apps, it would help catch bugs like espruino/BangleApps#2869

gfwilliams commented 1 year ago

So just to be clear, this is meant to allow this:

Bangle.setUI({remove : ... });

If so, I don't believe we should be allowing that. It was never intended behaviour, and isn't documented. I think if you made that call you might rightly assume that the remove function would be called when setUI was next called, but as far as I can see it's not - remove is just ignored:

https://github.com/espruino/Espruino/blob/master/libs/js/banglejs/Bangle_setUI_Q3.js

I think probably the intended use in that case is Bangle.setUI({mode:"custom",remove : ... });

bobrippling commented 1 year ago

Yes that makes sense and I agree, an object with no mode doesn't make sense.

My concern was that if someone writes their own setUI, they would assume that if there's an object, it'll have a valid mode, but we saw in espruino/BangleApps#2871 that this isn't necessarily the case - we might get passed an object without mode, and I wanted to guard against this kind of bug, if you see where I'm coming from?

(so less "documented" behaviour and more reflecting what existing apps already do)

gfwilliams commented 1 year ago

we might get passed an object without mode

Yes, I see what you mean (if you're implementing setUI). However adding an exception would encourage users (of which there are far more) to think omitting mode is ok. I think we should just fix messagelist since I believe it is not just mis-using setUI, but is broken. I did a quick check and as far as I can see messagelist is the only app doing it that way too.

And maybe even make the default .setUI throw an exception to stop people miss-using it.

I've just fixed messagelist so hopefully we can close this now?

bobrippling commented 1 year ago

Sounds good to me, and yeah that's a fair point about not encouraging that invalid use - thanks again.

(if you like, I'm happy to submit a patch to setUI and have it throw an exception if we get an object without mode)

gfwilliams commented 1 year ago

if you like, I'm happy to submit a patch to setUI and have it throw an exception if we get an object without mode

That would be great, thanks!