From ed7fdb3f235352592051dc5cda69d261cbe1fa82 Mon Sep 17 00:00:00 2001 From: Malian De Ron Date: Sat, 8 May 2021 15:23:54 +0200 Subject: [PATCH 1/9] Add :root as a valid option for prop --- lib/surface/api.ex | 2 +- test/api_test.exs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/surface/api.ex b/lib/surface/api.ex index 9dd3e0af..53dd245e 100644 --- a/lib/surface/api.ex +++ b/lib/surface/api.ex @@ -371,7 +371,7 @@ defmodule Surface.API do end defp get_valid_opts(:prop, _type, _opts) do - [:required, :default, :values, :values!, :accumulate] + [:required, :default, :values, :values!, :accumulate, :root] end defp get_valid_opts(:data, _type, _opts) do diff --git a/test/api_test.exs b/test/api_test.exs index 70f29918..4d78ad5b 100644 --- a/test/api_test.exs +++ b/test/api_test.exs @@ -289,7 +289,7 @@ defmodule Surface.APITest do code = "prop label, :string, a: 1" message = - ~r/unknown option :a. Available options: \[:required, :default, :values, :values!, :accumulate\]/ + ~r/unknown option :a. Available options: \[:required, :default, :values, :values!, :accumulate, :root\]/ assert_raise(CompileError, message, fn -> eval(code) From 922bda1c2c9319080cf29c12f98a1e5f0bb69d13 Mon Sep 17 00:00:00 2001 From: Malian De Ron Date: Sat, 8 May 2021 19:18:23 +0200 Subject: [PATCH 2/9] Introduce the concept of root properties --- lib/surface/api.ex | 41 +++++++++++++++++++--- lib/surface/compiler.ex | 50 ++++++++++++++++++++------- test/api_test.exs | 23 ++++++++++++ test/properties_test.exs | 75 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 16 deletions(-) diff --git a/lib/surface/api.ex b/lib/surface/api.ex index 53dd245e..d0bfef3e 100644 --- a/lib/surface/api.ex +++ b/lib/surface/api.ex @@ -59,6 +59,8 @@ defmodule Surface.API do # Any caller component can hold other components with slots Module.register_attribute(__MODULE__, :assigned_slots_by_parent, accumulate: false) + Module.register_attribute(__MODULE__, :root_prop, accumulate: false) + Module.put_attribute(__MODULE__, :use_context?, false) for func <- unquote(include) do @@ -114,6 +116,22 @@ defmodule Surface.API do } assigns = Module.get_attribute(caller.module, :assigns) || %{} + + validate_existing_assign!(assign, assigns, caller) + + if Keyword.get(opts, :root, false) do + validate_existing_root_prop!(assign, caller) + Module.put_attribute(caller.module, :root_prop, assign) + end + + name = Keyword.get(assign.opts, :as, assign.name) + new_assigns = Map.put(assigns, name, assign) + + Module.put_attribute(caller.module, :assigns, new_assigns) + Module.put_attribute(caller.module, assign.func, assign) + end + + defp validate_existing_assign!(assign, assigns, caller) do name = Keyword.get(assign.opts, :as, assign.name) existing_assign = assigns[name] @@ -125,12 +143,22 @@ defmodule Surface.API do message = ~s(cannot use name "#{assign.name}". #{details}.) IOHelper.compile_error(message, caller.file, assign.line) - else - assigns = Map.put(assigns, name, assign) - Module.put_attribute(caller.module, :assigns, assigns) end + end - Module.put_attribute(caller.module, assign.func, assign) + defp validate_existing_root_prop!(_assign, caller) do + root_prop = Module.get_attribute(caller.module, :root_prop) + + if root_prop do + message = """ + prop `label` is declared as a root property but another property \ + has also been declared has root property + + Hint: remove `root: true` from the properties that you don't want to use as root property + """ + + IOHelper.compile_error(message, caller.file, caller.line) + end end defp existing_assign_details_message(true = _builtin?, %{func: func}) do @@ -403,6 +431,11 @@ defmodule Surface.API do value end + defp validate_opt(:prop, _name, _type, _opts, :root, value, _caller) + when not is_boolean(value) do + {:error, "invalid value for option :root. Expected a boolean, got: #{inspect(value)}"} + end + defp validate_opt(_func, _name, _type, _opts, :required, value, _caller) when not is_boolean(value) do {:error, "invalid value for option :required. Expected a boolean, got: #{inspect(value)}"} diff --git a/lib/surface/compiler.ex b/lib/surface/compiler.ex index 13c65eff..021484d7 100644 --- a/lib/surface/compiler.ex +++ b/lib/surface/compiler.ex @@ -604,6 +604,24 @@ defmodule Surface.Compiler do |> Enum.reverse() end + defp process_attributes(mod, [{:root, value, attr_meta} | attrs], meta, acc) do + with true <- function_exported?(mod, :__props__, 0), + prop when not is_nil(prop) <- Enum.find(mod.__props__(), & &1.opts[:root]) do + name = Atom.to_string(prop.name) + process_attributes(mod, [{name, value, attr_meta} | attrs], meta, acc) + else + _ -> + message = """ + No root property for component <#{meta.node_alias}> + + Hint: declare a root property with `root: true` + """ + + IOHelper.warn(message, meta.caller, fn _ -> meta.line end) + process_attributes(mod, attrs, meta, acc) + end + end + defp process_attributes(mod, [{name, value, attr_meta} | attrs], meta, acc) do unquoted_string? = attr_meta[:unquoted_string?] name = String.to_atom(name) @@ -613,21 +631,29 @@ defmodule Surface.Compiler do accumulate? = Keyword.get(type_opts, :accumulate, false) if not accumulate? and Keyword.has_key?(acc, name) do - IOHelper.warn( - """ - The prop `#{name}` has been passed multiple times. Considering only the last value. + message = + if Keyword.get(type_opts, :root, false) do + """ + The prop `#{name}` has been passed multiple times. Considering only the last value. + + Hint: Either specify the `#{name}` via the root property (`<#{meta.node_alias} { ... }>`) or \ + explicitly via the #{name} property (`<#{meta.node_alias} #{name}="...">`), but not both. + """ + else + """ + The prop `#{name}` has been passed multiple times. Considering only the last value. + + Hint: Either remove all redundant definitions or set option `accumulate` to `true`: - Hint: Either remove all redundant definitions or set option `accumulate` to `true`: + ``` + prop #{name}, :#{type}, accumulate: true + ``` - ``` - prop #{name}, :#{type}, accumulate: true - ``` + This way the values will be accumulated in a list. + """ + end - This way the values will be accumulated in a list. - """, - meta.caller, - fn _ -> attr_meta.line end - ) + IOHelper.warn(message, meta.caller, fn _ -> attr_meta.line end) end if unquoted_string? do diff --git a/test/api_test.exs b/test/api_test.exs index 4d78ad5b..a9831f6b 100644 --- a/test/api_test.exs +++ b/test/api_test.exs @@ -66,6 +66,13 @@ defmodule Surface.APITest do assert_raise(CompileError, message, fn -> eval(code) end) end + test "validate :root in prop" do + code = "prop label, :string, root: 1" + message = ~r/invalid value for option :root. Expected a boolean, got: 1/ + + assert_raise(CompileError, message, fn -> eval(code) end) + end + test "validate duplicate assigns" do code = """ prop label, :string @@ -247,6 +254,22 @@ defmodule Surface.APITest do {:ok, _module} = eval(code, "LiveView") end + test "raise compile error for component with multiple root properties" do + code = """ + prop title, :string, root: true + prop label, :string, root: true + """ + + message = ~r""" + prop `label` is declared as a root property but another property \ + has also been declared has root property + + Hint: remove `root: true` from the properties that you don't want to use as root property + """ + + assert_raise(CompileError, message, fn -> eval(code) end) + end + test "accept invalid quoted expressions like literal maps as default value" do code = """ prop map, :map, default: %{a: 1, b: 2} diff --git a/test/properties_test.exs b/test/properties_test.exs index 2351bacb..03652d67 100644 --- a/test/properties_test.exs +++ b/test/properties_test.exs @@ -101,6 +101,18 @@ defmodule Surface.PropertiesTest do end end + defmodule RootProp do + use Surface.Component + + prop label, :string, root: true + + def render(assigns) do + ~H""" + { @label } + """ + end + end + describe "string" do test "passing a string literal" do assigns = %{text: "text"} @@ -596,6 +608,23 @@ defmodule Surface.PropertiesTest do """ end end + + describe "root property" do + test "component accepts root property" do + assigns = %{label: "Label"} + + html = + render_surface do + ~H""" + + """ + end + + assert html =~ """ + Label + """ + end + end end defmodule Surface.PropertiesSyncTest do @@ -603,6 +632,7 @@ defmodule Surface.PropertiesSyncTest do import ExUnit.CaptureIO alias Surface.PropertiesTest.StringProp, warn: false + alias Surface.PropertiesTest.RootProp, warn: false test "warn if prop is required and has default value" do id = :erlang.unique_integer([:positive]) |> to_string() @@ -763,4 +793,49 @@ defmodule Surface.PropertiesSyncTest do prop `invalid_acc2` default value `3` must be a list when `accumulate: true` """ end + + test "warn if component does not accept a root property" do + assigns = %{label: "root"} + + code = + quote do + ~H""" + + """ + end + + output = + capture_io(:standard_error, fn -> + compile_surface(code, assigns) + end) + + assert output =~ ~r""" + No root property for component + + Hint: declare a root property with `root: true` + """ + end + + test "warn if tag has a root property and the property assigned normally" do + assigns = %{label: "root"} + + code = + quote do + ~H""" + + """ + end + + output = + capture_io(:standard_error, fn -> + compile_surface(code, assigns) + end) + + assert output =~ ~r""" + The prop `label` has been passed multiple times. Considering only the last value. + + Hint: Either specify the `label` via the root property \(``\) or \ + explicitly via the label property \(``\), but not both. + """ + end end From 0e4b40a970edec29f15f1dc8aec58be774a0f6e3 Mon Sep 17 00:00:00 2001 From: Malian De Ron Date: Sun, 9 May 2021 08:48:01 +0200 Subject: [PATCH 3/9] Remove hardcoded value --- lib/surface/api.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/surface/api.ex b/lib/surface/api.ex index d0bfef3e..d21859ad 100644 --- a/lib/surface/api.ex +++ b/lib/surface/api.ex @@ -146,12 +146,12 @@ defmodule Surface.API do end end - defp validate_existing_root_prop!(_assign, caller) do + defp validate_existing_root_prop!(assign, caller) do root_prop = Module.get_attribute(caller.module, :root_prop) if root_prop do message = """ - prop `label` is declared as a root property but another property \ + prop `#{assign.name}` is declared as a root property but another property \ has also been declared has root property Hint: remove `root: true` from the properties that you don't want to use as root property From b17ca7d6e1b7d75a239173e6c8040aa9b428cac5 Mon Sep 17 00:00:00 2001 From: Malian De Ron Date: Sun, 9 May 2021 09:06:00 +0200 Subject: [PATCH 4/9] Add test for an AST with a root property --- test/compiler_test.exs | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/test/compiler_test.exs b/test/compiler_test.exs index ce66d4cb..83d25869 100644 --- a/test/compiler_test.exs +++ b/test/compiler_test.exs @@ -30,7 +30,7 @@ defmodule Surface.CompilerTest do defmodule Button do use Surface.Component - prop label, :string, default: "" + prop label, :string, default: "", root: true prop click, :event prop class, :css_class prop disabled, :boolean @@ -205,6 +205,27 @@ defmodule Surface.CompilerTest do } = node end + test "component with root property" do + code = """ +