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

Make the smart cell editor source explicitly managed #391

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

jonatanklosko
Copy link
Member

@jonatanklosko jonatanklosko commented Jan 31, 2024

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
Copy link
Member Author

@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
Copy link
Contributor

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

@josevalim
Copy link
Contributor

Just temporarily as part of the deprecation period.

@jonatanklosko
Copy link
Member Author

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
Copy link
Contributor

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

@jonatanklosko
Copy link
Member Author

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
Copy link
Contributor

@jonatanklosko yes!

@josevalim
Copy link
Contributor

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
Copy link
Contributor

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

@jonatanklosko
Copy link
Member Author

jonatanklosko commented Feb 1, 2024

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
Copy link
Contributor

@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
Copy link
Member Author

Callback it is :) Updated!

@jonatanklosko jonatanklosko merged commit 29839b7 into main Feb 1, 2024
1 check passed
@jonatanklosko jonatanklosko deleted the jk-smart-cell-editor-source branch February 1, 2024 09:16
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.

2 participants