livebook-dev / kino

Client-driven interactive widgets for Livebook
Apache License 2.0
369 stars 65 forks source link

Make the smart cell editor source explicitly managed #391

Closed jonatanklosko closed 9 months ago

jonatanklosko commented 9 months ago

This is a breaking change, but I think it's better to do it sooner than later. Currently the user specifies under which attr the editor source should be placed and it is only available inside the to_source(attrs) callback. This PR makes the flow more explicit, we expose a new callback handle_editor_change(source, ctx) and it is up to the user to keep it in assigns and put in attrs. This could be used to update the UI based on the editor source (for example to parse the code and show errors).

Note that this is a breaking change only in that the smart cell implementation needs to be updated (a simple change). It doesn't change anything on Livebook side, and smart cell serialization is also not affected (provided the same attribute is used).

jonatanklosko commented 9 months ago

@josevalim kino_db has {:kino, "~> 0.7"}, I would release one version that depends on <= 0.12, and then another that requires 0.13 (once released). Does that make sense?

josevalim commented 9 months ago

@jonatanklosko what if we detect if handle_editor_change is defined and, if not, we fallback to old behaviour?

josevalim commented 9 months ago

Just temporarily as part of the deprecation period.

jonatanklosko commented 9 months ago

We could detect based on whether they configure using :source or :attribute. But I'm not sure it's worth supporting both behaviours, given it's a fairly rare use case and a pretty simple change to adapt, especially if 0.13 is going to be the last one before 1.0. The kino_db case is still the same either way.

josevalim commented 9 months ago

Having it, even for one version, will aid with breaking changes, so I think we should do it. :)

jonatanklosko commented 9 months ago

Ok, so the current kino_db will work under the deprecation and we only release next version depending on ~> 0.13, and do 1.0 once we have kino 1.0?

josevalim commented 9 months ago

@jonatanklosko yes!

josevalim commented 9 months ago

Btw, do you think it is worth a new callback? Could we just send it as a regular event (perhaps make the event name configurable under the new API?)?

josevalim commented 9 months ago

Well, we need the new callback for the deprecation, so nevermind. :D

jonatanklosko commented 9 months ago

We will deprecate based on the attributes, previously [editor: [attribute: "..."]] was required and now [editor: [source: "..."]] is required.

The main difference with the callback is that we can error sooner (on smart cell startup) if the user enables editor and doesn't define the callback, but either works for me, so whichever you like more :)

josevalim commented 9 months ago

@jonatanklosko I have no preference. Both options are fine. Given that the smart cell is mostly optional callbacks, keeping it as an optional callback is also easier to discover, so it probably justifies it. But it is your call. :)

jonatanklosko commented 9 months ago

Callback it is :) Updated!