From c1a6bc1aa86829a11754506ad66f8c312c55e689 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Wed, 27 Oct 2021 13:35:24 +0200 Subject: [PATCH] Remove reactive inputs (#649) --- lib/livebook/live_markdown/export.ex | 4 +- lib/livebook/live_markdown/import.ex | 16 +++-- lib/livebook/notebook/cell/input.ex | 20 +----- lib/livebook/session/data.ex | 22 ------- .../input_cell_settings_component.ex | 9 +-- test/livebook/live_markdown/export_test.exs | 5 +- test/livebook/live_markdown/import_test.exs | 21 +++++- test/livebook/notebook/cell/input_test.exs | 46 ------------- test/livebook/notebook/export/elixir_test.exs | 3 +- test/livebook/session/data_test.exs | 64 ------------------- 10 files changed, 38 insertions(+), 172 deletions(-) diff --git a/lib/livebook/live_markdown/export.ex b/lib/livebook/live_markdown/export.ex index c53b7f38a..d236b6497 100644 --- a/lib/livebook/live_markdown/export.ex +++ b/lib/livebook/live_markdown/export.ex @@ -111,8 +111,8 @@ defmodule Livebook.LiveMarkdown.Export do value: value } |> put_unless_default( - Map.take(cell, [:reactive, :props]), - Map.take(Cell.Input.new(), [:reactive, :props]) + Map.take(cell, [:props]), + Map.take(Cell.Input.new(), [:props]) ) |> Jason.encode!() diff --git a/lib/livebook/live_markdown/import.ex b/lib/livebook/live_markdown/import.ex index e90aaa601..81ee7312d 100644 --- a/lib/livebook/live_markdown/import.ex +++ b/lib/livebook/live_markdown/import.ex @@ -213,9 +213,9 @@ defmodule Livebook.LiveMarkdown.Import do defp build_notebook([{:cell, :input, data} | elems], cells, sections, messages) do case parse_input_attrs(data) do - {:ok, attrs} -> + {:ok, attrs, input_messages} -> cell = Notebook.Cell.new(:input) |> Map.merge(attrs) - build_notebook(elems, [cell | cells], sections, messages) + build_notebook(elems, [cell | cells], sections, messages ++ input_messages) {:error, message} -> build_notebook(elems, cells, sections, [message | messages]) @@ -273,15 +273,23 @@ defmodule Livebook.LiveMarkdown.Import do defp parse_input_attrs(data) do with {:ok, type} <- parse_input_type(data["type"]) do + warnings = + if data["reactive"] == true do + [ + "found a reactive input, but those are no longer supported, you can use automatically reevaluating cell instead" + ] + else + [] + end + {:ok, %{ type: type, name: data["name"], value: data["value"], # Fields with implicit value - reactive: Map.get(data, "reactive", false), props: data |> Map.get("props", %{}) |> parse_input_props(type) - }} + }, warnings} end end diff --git a/lib/livebook/notebook/cell/input.ex b/lib/livebook/notebook/cell/input.ex index 25864413a..9a8238446 100644 --- a/lib/livebook/notebook/cell/input.ex +++ b/lib/livebook/notebook/cell/input.ex @@ -6,7 +6,7 @@ defmodule Livebook.Notebook.Cell.Input do # It consists of an input that the user may fill # and then read during code evaluation. - defstruct [:id, :type, :name, :value, :reactive, :props] + defstruct [:id, :type, :name, :value, :props] alias Livebook.Utils alias Livebook.Notebook.Cell @@ -16,7 +16,6 @@ defmodule Livebook.Notebook.Cell.Input do type: type(), name: String.t(), value: String.t(), - reactive: boolean(), props: props() } @@ -39,7 +38,6 @@ defmodule Livebook.Notebook.Cell.Input do type: :text, name: "input", value: "", - reactive: false, props: %{} } end @@ -100,22 +98,6 @@ defmodule Livebook.Notebook.Cell.Input do def default_props(:select), do: %{options: [""]} def default_props(_type), do: %{} - @doc """ - Checks if the input changed in terms of content. - """ - @spec invalidated?(t(), t()) :: boolean() - def invalidated?(cell, prev_cell) do - cell.value != prev_cell.value or cell.name != prev_cell.name - end - - @doc """ - Checks if the input change should trigger reactive update. - """ - @spec reactive_update?(t(), t()) :: boolean() - def reactive_update?(cell, prev_cell) do - cell.reactive and cell.value != prev_cell.value and validate(cell) == :ok - end - @doc """ Parses input type from string. """ diff --git a/lib/livebook/session/data.ex b/lib/livebook/session/data.ex index fbaa5f3fc..0fc18434e 100644 --- a/lib/livebook/session/data.ex +++ b/lib/livebook/session/data.ex @@ -541,11 +541,6 @@ defmodule Livebook.Session.Data do data |> with_actions() |> set_cell_attributes(cell, attrs) - |> then(fn {data, _} = data_actions -> - {:ok, updated_cell, _} = Notebook.fetch_cell_and_section(data.notebook, cell_id) - maybe_queue_bound_cells(data_actions, updated_cell, cell) - end) - |> maybe_evaluate_queued() |> compute_snapshots_and_validity() |> set_dirty() |> wrap_ok() @@ -1141,23 +1136,6 @@ defmodule Livebook.Session.Data do |> set!(notebook: Notebook.update_cell(data.notebook, cell.id, &Map.merge(&1, attrs))) end - defp maybe_queue_bound_cells({data, _} = data_actions, %Cell.Input{} = cell, prev_cell) do - if Cell.Input.reactive_update?(cell, prev_cell) do - bound_cells = bound_cells_with_section(data, cell.id) - - data_actions - |> reduce(bound_cells, fn data_actions, {bound_cell, section} -> - data_actions - |> queue_prerequisite_cells_evaluation(bound_cell) - |> queue_cell_evaluation(bound_cell, section) - end) - else - data_actions - end - end - - defp maybe_queue_bound_cells(data_actions, _cell, _prev_cell), do: data_actions - defp set_runtime(data_actions, prev_data, runtime) do {data, _} = data_actions = set!(data_actions, runtime: runtime) diff --git a/lib/livebook_web/live/session_live/input_cell_settings_component.ex b/lib/livebook_web/live/session_live/input_cell_settings_component.ex index 939c989d5..171c70dae 100644 --- a/lib/livebook_web/live/session_live/input_cell_settings_component.ex +++ b/lib/livebook_web/live/session_live/input_cell_settings_component.ex @@ -13,7 +13,7 @@ defmodule LivebookWeb.SessionLive.InputCellSettingsComponent do |> assign(assigns) |> assign(:current_type, cell.type) |> assign_new(:attrs, fn -> - Map.take(cell, [:name, :type, :reactive, :props]) + Map.take(cell, [:name, :type, :props]) end) |> assign_new(:valid, fn -> true end) @@ -43,10 +43,6 @@ defmodule LivebookWeb.SessionLive.InputCellSettingsComponent do <.extra_fields type={@attrs.type} props={@attrs.props} myself={@myself} /> - <.switch_checkbox - name="attrs[reactive]" - label="Reactive (reevaluates dependent cells on change)" - checked={@attrs.reactive} />
<%= live_patch "Cancel", to: @return_to, class: "button button-outlined-gray" %> @@ -161,7 +157,6 @@ defmodule LivebookWeb.SessionLive.InputCellSettingsComponent do defp validate_attrs(data, prev_attrs) do name = data["name"] type = data["type"] |> String.to_existing_atom() - reactive = data["reactive"] == "true" {props_valid?, props} = if type == prev_attrs.type do @@ -172,7 +167,7 @@ defmodule LivebookWeb.SessionLive.InputCellSettingsComponent do valid? = name != "" and props_valid? - {valid?, %{name: name, type: type, reactive: reactive, props: props}} + {valid?, %{name: name, type: type, props: props}} end defp validate_props(data, :range) do diff --git a/test/livebook/live_markdown/export_test.exs b/test/livebook/live_markdown/export_test.exs index dbfac1938..88bd722d5 100644 --- a/test/livebook/live_markdown/export_test.exs +++ b/test/livebook/live_markdown/export_test.exs @@ -50,8 +50,7 @@ defmodule Livebook.LiveMarkdown.ExportTest do Notebook.Cell.new(:input) | type: :text, name: "length", - value: "100", - reactive: true + value: "100" }, %{ Notebook.Cell.new(:elixir) @@ -107,7 +106,7 @@ defmodule Livebook.LiveMarkdown.ExportTest do ## Section 2 - + ```elixir IO.gets("length: ") diff --git a/test/livebook/live_markdown/import_test.exs b/test/livebook/live_markdown/import_test.exs index 8c1d01b4b..b803337a4 100644 --- a/test/livebook/live_markdown/import_test.exs +++ b/test/livebook/live_markdown/import_test.exs @@ -29,7 +29,7 @@ defmodule Livebook.LiveMarkdown.ImportTest do ## Section 2 - + ```elixir IO.gets("length: ") @@ -88,8 +88,7 @@ defmodule Livebook.LiveMarkdown.ImportTest do %Cell.Input{ type: :text, name: "length", - value: "100", - reactive: true + value: "100" }, %Cell.Elixir{ source: """ @@ -598,4 +597,20 @@ defmodule Livebook.LiveMarkdown.ImportTest do ~s{unrecognised input type "input_from_the_future", if it's a valid type it means your Livebook version doesn't support it} ] == messages end + + describe "backward compatibility" do + markdown = """ + # My Notebook + + ## Section 1 + + + """ + + {_notebook, messages} = Import.notebook_from_markdown(markdown) + + assert [ + "found a reactive input, but those are no longer supported, you can use automatically reevaluating cell instead" + ] == messages + end end diff --git a/test/livebook/notebook/cell/input_test.exs b/test/livebook/notebook/cell/input_test.exs index 59585ece8..eb31ec596 100644 --- a/test/livebook/notebook/cell/input_test.exs +++ b/test/livebook/notebook/cell/input_test.exs @@ -68,50 +68,4 @@ defmodule Livebook.Notebook.Cell.InputText do assert Input.validate(input) == {:error, "number too big"} end end - - describe "invalidated?/2" do - test "returns false if only the type changes" do - input = %{Input.new() | type: :text} - updated_input = %{input | type: :url} - - refute Input.invalidated?(updated_input, input) - end - - test "returns true if the name changes" do - input = %{Input.new() | name: "Name"} - updated_input = %{input | name: "Full name"} - - assert Input.invalidated?(updated_input, input) - end - - test "returns true if the value changes" do - input = %{Input.new() | value: "Jake Peralta"} - updated_input = %{input | value: "Amy Santiago"} - - assert Input.invalidated?(updated_input, input) - end - end - - describe "reactive_change?/2" do - test "returns false if the input is not reactive" do - input = %{Input.new() | reactive: false, value: "Jake Peralta"} - updated_input = %{input | value: "Amy Santiago"} - - refute Input.reactive_update?(updated_input, input) - end - - test "returns true if the input is reactive and value changes" do - input = %{Input.new() | reactive: true, value: "Jake Peralta"} - updated_input = %{input | value: "Amy Santiago"} - - assert Input.reactive_update?(updated_input, input) - end - - test "returns false if the new value is invalid" do - input = %{Input.new() | reactive: true, type: :number, value: "10"} - updated_input = %{input | value: "invalid"} - - refute Input.reactive_update?(updated_input, input) - end - end end diff --git a/test/livebook/notebook/export/elixir_test.exs b/test/livebook/notebook/export/elixir_test.exs index b0c3f1aa4..e5b281e55 100644 --- a/test/livebook/notebook/export/elixir_test.exs +++ b/test/livebook/notebook/export/elixir_test.exs @@ -47,8 +47,7 @@ defmodule Livebook.Notebook.Export.ElixirTest do Notebook.Cell.new(:input) | type: :text, name: "length", - value: "100", - reactive: true + value: "100" }, %{ Notebook.Cell.new(:elixir) diff --git a/test/livebook/session/data_test.exs b/test/livebook/session/data_test.exs index c7c7531f7..4a86500b5 100644 --- a/test/livebook/session/data_test.exs +++ b/test/livebook/session/data_test.exs @@ -2933,7 +2933,6 @@ defmodule Livebook.Session.DataTest do data_after_operations!([ {:insert_section, self(), 0, "s1"}, {:insert_cell, self(), "s1", 0, :input, "c1"}, - {:set_cell_attributes, self(), "c1", %{reactive: false}}, # Insert three evaluated cells and bind the second one to the input {:insert_cell, self(), "s1", 1, :elixir, "c2"}, {:insert_cell, self(), "s1", 2, :elixir, "c3"}, @@ -2961,69 +2960,6 @@ defmodule Livebook.Session.DataTest do }, _} = Data.apply_operation(data, operation) end - test "given reactive input value change, triggers bound cells evaluation" do - data = - data_after_operations!([ - {:insert_section, self(), 0, "s1"}, - {:insert_cell, self(), "s1", 0, :input, "c1"}, - {:set_cell_attributes, self(), "c1", %{reactive: true}}, - # Insert three evaluated cells and bind the second and third one to the input - {:insert_cell, self(), "s1", 1, :elixir, "c2"}, - {:insert_cell, self(), "s1", 2, :elixir, "c3"}, - {:insert_cell, self(), "s1", 3, :elixir, "c4"}, - {:set_runtime, self(), NoopRuntime.new()}, - {:queue_cell_evaluation, self(), "c2"}, - {:queue_cell_evaluation, self(), "c3"}, - {:queue_cell_evaluation, self(), "c4"}, - {:add_cell_evaluation_response, self(), "c2", @eval_resp, @eval_meta}, - {:add_cell_evaluation_response, self(), "c3", @eval_resp, @eval_meta}, - {:add_cell_evaluation_response, self(), "c4", @eval_resp, @eval_meta}, - {:bind_input, self(), "c3", "c1"}, - {:bind_input, self(), "c4", "c1"} - ]) - - attrs = %{value: "stuff"} - operation = {:set_cell_attributes, self(), "c1", attrs} - - assert {:ok, - %{ - cell_infos: %{ - "c2" => %{evaluation_status: :ready}, - "c3" => %{evaluation_status: :evaluating}, - "c4" => %{evaluation_status: :queued} - } - }, _} = Data.apply_operation(data, operation) - end - - test "given reactive input value change, queues bound cell evaluation even if evaluating" do - data = - data_after_operations!([ - {:insert_section, self(), 0, "s1"}, - {:insert_cell, self(), "s1", 0, :input, "c1"}, - {:set_cell_attributes, self(), "c1", %{reactive: true}}, - {:insert_cell, self(), "s1", 1, :elixir, "c2"}, - {:set_runtime, self(), NoopRuntime.new()}, - {:queue_cell_evaluation, self(), "c2"}, - {:bind_input, self(), "c2", "c1"} - ]) - - attrs = %{value: "stuff"} - operation = {:set_cell_attributes, self(), "c1", attrs} - - assert {:ok, - %{ - cell_infos: %{ - "c2" => %{evaluation_status: :evaluating} - }, - section_infos: %{ - "s1" => %{ - evaluating_cell_id: "c2", - evaluation_queue: ["c2"] - } - } - }, _} = Data.apply_operation(data, operation) - end - test "setting reevaluate_automatically on stale cell marks it for evaluation" do data = data_after_operations!([