-
Notifications
You must be signed in to change notification settings - Fork 65
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
Conversation
@josevalim |
@jonatanklosko what if we detect if |
Just temporarily as part of the deprecation period. |
We could detect based on whether they configure using |
Having it, even for one version, will aid with breaking changes, so I think we should do it. :) |
Ok, so the current |
@jonatanklosko yes! |
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?)? |
Well, we need the new callback for the deprecation, so nevermind. :D |
We will deprecate based on the attributes, previously 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 :) |
@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. :) |
Callback it is :) Updated! |
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 theto_source(attrs)
callback. This PR makes the flow more explicit, we expose a new callbackhandle_editor_change(source, ctx)
and it is up to the user to keep it in assigns and put inattrs
. 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).