-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
New practice exercise react
#820
Changes from 9 commits
4eb93dd
45913e7
9989015
650a34b
c876cd2
5dfadb0
02e436b
d3c1848
968a607
9e261dd
607bc10
d7d0a6a
4762a7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
# Description | ||
|
||
Implement a basic reactive system. | ||
|
||
Reactive programming is a programming paradigm that focuses on how values | ||
are computed in terms of each other to allow a change to one value to | ||
automatically propagate to other values, like in a spreadsheet. | ||
|
||
Implement a basic reactive system with cells with settable values ("input" | ||
cells) and cells with values computed in terms of other cells ("compute" | ||
cells). Implement updates so that when an input value is changed, values | ||
propagate to reach a new stable system state. | ||
|
||
In addition, compute cells should allow for registering change notification | ||
callbacks. Call a cell’s callbacks when the cell’s value in a new stable | ||
state has changed from the previous stable state. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# Used by "mix format" | ||
[ | ||
inputs: ["{mix,.formatter}.exs", "{config,lib,test}/**/*.{ex,exs}"] | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
{ | ||
"authors": [ "jiegillet" ], | ||
"contributors": [ | ||
"angelikatyborska", | ||
"neenjaw" | ||
], | ||
"files": { | ||
"example": [ | ||
".meta/example.ex" | ||
], | ||
"solution": [ | ||
"lib/react.ex" | ||
], | ||
"test": [ | ||
"test/react_test.exs" | ||
] | ||
}, | ||
"blurb": "Implement a basic reactive system." | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# Design Decisions | ||
|
||
## Strings as identifiers | ||
|
||
We have decided to use strings rather than atoms to identify the cells. The reason is the one mentioned in the [Elixir Guide on GenServers](https://elixir-lang.org/getting-started/mix-otp/genserver.html) about naming processes: atoms are not garbage collected and should not be generated from user input. | ||
|
||
The cells in this exercise are not generated in such a way, but it seems like they would be in a production setting, every time the user would request more resources. | ||
|
||
## Custom tests | ||
|
||
As encouraged by the problem specifications, we have added tests using different types of value. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,171 @@ | ||
defmodule React do | ||
use GenServer | ||
|
||
defmodule InputCell do | ||
defstruct [:name, :value] | ||
@type t :: %InputCell{name: String.t(), value: any} | ||
end | ||
|
||
defmodule OutputCell do | ||
defstruct [:name, :inputs, :compute, :value, callbacks: %{}] | ||
|
||
@type t :: %OutputCell{ | ||
name: String.t(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ooops, didn't see the PR description before posting. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree with the concern, let's keep strings. Maybe you could describe this in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good suggestion, I will do that. Those notes are only meant for maintainers right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes |
||
inputs: [String.t()], | ||
compute: fun(), | ||
value: any, | ||
callbacks: %{String.t() => (String.t(), any -> :ok)} | ||
} | ||
end | ||
|
||
# CLIENT SIDE | ||
|
||
@doc """ | ||
Start a reactive system | ||
""" | ||
@spec new(cells :: [%{atom => any}]) :: {:ok, pid} | ||
def new(cells) do | ||
GenServer.start_link(React, cells) | ||
end | ||
|
||
@doc """ | ||
Return the value of an input or output cell | ||
""" | ||
@spec get_value(cells :: pid, cell :: String.t()) :: any() | ||
def get_value(cells, cell) do | ||
GenServer.call(cells, {:get_value, cell}) | ||
end | ||
|
||
@doc """ | ||
Set the value of an input cell | ||
""" | ||
@spec set_value(cells :: pid, cell :: String.t(), value :: any) :: :ok | ||
def set_value(cells, cell, value) do | ||
GenServer.cast(cells, {:set_value, cell, value}) | ||
end | ||
|
||
@doc """ | ||
Add a callback to an output cell | ||
""" | ||
@spec add_callback( | ||
cells :: pid, | ||
cell :: String.t(), | ||
callback :: String.t(), | ||
send :: (String.t(), any -> :ok) | ||
) :: :ok | ||
def add_callback(cells, cell, callback, send) do | ||
GenServer.cast(cells, {:add_callback, cell, callback, send}) | ||
end | ||
|
||
@doc """ | ||
Remove a callback from an output cell | ||
""" | ||
@spec remove_callback(cells :: pid, cell :: String.t(), callback :: String.t()) :: :ok | ||
def remove_callback(cells, cell, callback) do | ||
GenServer.cast(cells, {:remove_callback, cell, callback}) | ||
end | ||
|
||
# SERVER SIDE | ||
|
||
defmodule State do | ||
defstruct [:cells, :dependencies] | ||
end | ||
|
||
@impl true | ||
def init(cells) do | ||
cells = | ||
Map.new(cells, fn | ||
%{type: :input} = cell -> | ||
{cell.name, %InputCell{name: cell.name, value: cell.value}} | ||
|
||
%{type: :output} = cell -> | ||
{cell.name, %OutputCell{name: cell.name, inputs: cell.inputs, compute: cell.compute}} | ||
end) | ||
|
||
initialized_cells = | ||
Map.new(cells, fn {name, cell} -> {name, initialize_value(cell, cells)} end) | ||
|
||
dependencies = | ||
Enum.reduce(cells, %{}, fn | ||
{name, %OutputCell{inputs: [a]}}, deps -> | ||
Map.update(deps, a, [name], fn names -> [name | names] end) | ||
|
||
{name, %OutputCell{inputs: [a, b]}}, deps -> | ||
Map.update(deps, a, [name], fn names -> [name | names] end) | ||
|> Map.update(b, [name], fn names -> [name | names] end) | ||
|
||
_input, deps -> | ||
deps | ||
end) | ||
|
||
{:ok, %State{cells: initialized_cells, dependencies: dependencies}} | ||
end | ||
|
||
@impl true | ||
def handle_call({:get_value, name}, _from, %State{cells: cells} = state) do | ||
{:reply, cells[name].value, state} | ||
end | ||
|
||
@impl true | ||
def handle_cast({:set_value, name, value}, %State{cells: cells, dependencies: deps} = state) do | ||
%InputCell{} = input = cells[name] | ||
|
||
cells = | ||
Map.put(cells, name, %{input | value: value}) | ||
|> update_dependencies(deps[name], deps) | ||
|
||
{:noreply, %{state | cells: cells}} | ||
end | ||
|
||
@impl true | ||
def handle_cast({:add_callback, name, callback, send}, %State{cells: cells} = state) do | ||
%OutputCell{callbacks: callbacks} = cell = cells[name] | ||
callbacks = Map.put(callbacks, callback, send) | ||
{:noreply, %{state | cells: Map.put(cells, name, %{cell | callbacks: callbacks})}} | ||
end | ||
|
||
@impl true | ||
def handle_cast({:remove_callback, name, callback}, %State{cells: cells} = state) do | ||
%OutputCell{callbacks: callbacks} = cell = cells[name] | ||
callbacks = Map.delete(callbacks, callback) | ||
{:noreply, %{state | cells: Map.put(cells, name, %{cell | callbacks: callbacks})}} | ||
end | ||
|
||
defp initialize_value(%OutputCell{value: nil, inputs: [a], compute: f} = cell, cells) do | ||
reference = initialize_value(cells[a], cells) | ||
%{cell | value: f.(reference.value)} | ||
end | ||
|
||
defp initialize_value(%OutputCell{value: nil, inputs: [a, b], compute: f} = cell, cells) do | ||
reference_a = initialize_value(cells[a], cells) | ||
reference_b = initialize_value(cells[b], cells) | ||
%{cell | value: f.(reference_a.value, reference_b.value)} | ||
end | ||
|
||
defp initialize_value(cell, _cells), do: cell | ||
|
||
defp update_dependencies(cells, [name | to_update], dependencies) do | ||
cell = cells[name] | ||
|
||
value = | ||
case cell do | ||
%OutputCell{inputs: [a], compute: f} -> f.(cells[a].value) | ||
%OutputCell{inputs: [a, b], compute: f} -> f.(cells[a].value, cells[b].value) | ||
end | ||
|
||
cells = Map.put(cells, name, %{cell | value: value}) | ||
|
||
if(value == cell.value) do | ||
update_dependencies(cells, to_update, dependencies) | ||
else | ||
cells = Map.put(cells, cell, %{cell | value: value}) | ||
|
||
Enum.each(cell.callbacks, fn {name, send} -> send.(name, value) end) | ||
|
||
next = Map.get(dependencies, name, []) | ||
update_dependencies(cells, to_update ++ next, dependencies) | ||
end | ||
end | ||
|
||
defp update_dependencies(cells, _empty, _dependencies), do: cells | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
# This is an auto-generated file. | ||
# | ||
# Regenerating this file via `configlet sync` will: | ||
# - Recreate every `description` key/value pair | ||
# - Recreate every `reimplements` key/value pair, where they exist in problem-specifications | ||
# - Remove any `include = true` key/value pair (an omitted `include` key implies inclusion) | ||
# - Preserve any other key/value pair | ||
# | ||
# As user-added comments (using the # character) will be removed when this file | ||
# is regenerated, comments can be added via a `comment` key. | ||
[c51ee736-d001-4f30-88d1-0c8e8b43cd07] | ||
description = "input cells have a value" | ||
|
||
[dedf0fe0-da0c-4d5d-a582-ffaf5f4d0851] | ||
description = "an input cell's value can be set" | ||
|
||
[5854b975-f545-4f93-8968-cc324cde746e] | ||
description = "compute cells calculate initial value" | ||
|
||
[25795a3d-b86c-4e91-abe7-1c340e71560c] | ||
description = "compute cells take inputs in the right order" | ||
|
||
[c62689bf-7be5-41bb-b9f8-65178ef3e8ba] | ||
description = "compute cells update value when dependencies are changed" | ||
|
||
[5ff36b09-0a88-48d4-b7f8-69dcf3feea40] | ||
description = "compute cells can depend on other compute cells" | ||
|
||
[abe33eaf-68ad-42a5-b728-05519ca88d2d] | ||
description = "compute cells fire callbacks" | ||
|
||
[9e5cb3a4-78e5-4290-80f8-a78612c52db2] | ||
description = "callback cells only fire on change" | ||
|
||
[ada17cb6-7332-448a-b934-e3d7495c13d3] | ||
description = "callbacks do not report already reported values" | ||
|
||
[ac271900-ea5c-461c-9add-eeebcb8c03e5] | ||
description = "callbacks can fire from multiple cells" | ||
|
||
[95a82dcc-8280-4de3-a4cd-4f19a84e3d6f] | ||
description = "callbacks can be added and removed" | ||
|
||
[f2a7b445-f783-4e0e-8393-469ab4915f2a] | ||
description = "removing a callback multiple times doesn't interfere with other callbacks" | ||
|
||
[daf6feca-09e0-4ce5-801d-770ddfe1c268] | ||
description = "callbacks should only be called once even if multiple dependencies change" | ||
|
||
[9a5b159f-b7aa-4729-807e-f1c38a46d377] | ||
description = "callbacks should not be called if dependencies change but output value doesn't change" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
defmodule React do | ||
@doc """ | ||
Start a reactive system | ||
""" | ||
@spec new(cells :: [%{atom => any}]) :: {:ok, pid} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The map %{type: :input, name: String.t(), value: any()}
%{type: :output, name: String.t(), inputs: [String.t()], compute: (...) -> any()} Should we write that in the typespec instead of the generic |
||
def new(cells) do | ||
end | ||
|
||
@doc """ | ||
Return the value of an input or output cell | ||
""" | ||
@spec get_value(cells :: pid, cell :: String.t()) :: any() | ||
def get_value(cells, cell) do | ||
end | ||
|
||
@doc """ | ||
Set the value of an input cell | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this needs a hint about what the function returns? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, the callbacks. Yes, good idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After some thought, I find it a bit weird that there are no actual functions involved in the callbacks 🤔. React.add_callback(cells, "always_two", fn value -> send(self(), {:callback1, value}))
:ok = React.set_value(cells, "input", 2)
refute_received {:callback, _}, "Expected callback not to be called" # maybe the failure message can be improved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah nice! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had a go at something that I found sensible, please check :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks much better 👍 |
||
""" | ||
@spec set_value(cells :: pid, cell :: String.t(), value :: any) :: :ok | ||
def set_value(cells, cell, value) do | ||
end | ||
|
||
@doc """ | ||
Add a callback to an output cell | ||
""" | ||
@spec add_callback( | ||
cells :: pid, | ||
cell :: String.t(), | ||
callback :: String.t(), | ||
send :: (String.t(), any -> :ok) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it really matter what the callback returns? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well the callback must send a message, which is of type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Must send a message? The callback we use in tests sends a message so that we have an easy way of verifying that it was called, but in general the callback can do anything. It's even in your implementation:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I get it. I think what I was doing here is matching the type of the actually callbacks , which are all predefined in the tests as |
||
) :: :ok | ||
def add_callback(cells, cell, callback, send) do | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could improve argument names in the whole stub file E.g. I'm not entirely sure about the name "send". It's specific to message sending, but the callback can do anything. In practice it only makes sense if it does side-effect (because there's nobody that cares about the return value) but the side effects could be not only message sending, but also doing network requests or something 🤷 I would propose: |
||
|
||
@doc """ | ||
Remove a callback from an output cell | ||
""" | ||
@spec remove_callback(cells :: pid, cell :: String.t(), callback :: String.t()) :: :ok | ||
def remove_callback(cells, cell, callback) do | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
defmodule React.MixProject do | ||
use Mix.Project | ||
|
||
def project do | ||
[ | ||
app: :react, | ||
version: "0.1.0", | ||
# elixir: "~> 1.8", | ||
start_permanent: Mix.env() == :prod, | ||
deps: deps() | ||
] | ||
end | ||
|
||
# Run "mix help compile.app" to learn about applications. | ||
def application do | ||
[ | ||
extra_applications: [:logger] | ||
] | ||
end | ||
|
||
# Run "mix help deps" to learn about dependencies. | ||
defp deps do | ||
[ | ||
# {:dep_from_hexpm, "~> 0.3.0"}, | ||
# {:dep_from_git, git: "https://github.com/elixir-lang/my_dep.git", tag: "0.1.0"} | ||
] | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a note in #559 that this will be a genserver exercise once we have that concept 🙂