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

Support function components #473

Merged
merged 9 commits into from
Sep 6, 2021
Merged

Support function components #473

merged 9 commits into from
Sep 6, 2021

Conversation

msaraiva
Copy link
Member

@msaraiva msaraiva commented Aug 3, 2021

Also closes #343 and #24.

  * Support local and remote function components
  * Support recursive components by translating them into function components
  * No support for slots
  * No support for contexts
  * Don't validate args
@msaraiva msaraiva marked this pull request as ready for review September 4, 2021 12:58
Copy link
Collaborator

@lnr0626 lnr0626 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, good work!

I had a few small comments/questions, but no blockers

@@ -1223,4 +1294,17 @@ defmodule Surface.Compiler do

IOHelper.compile_error(message, file, line)
end

defp handle_convert_node_to_ast_error(name, error, meta) do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

caller_component_type = Module.get_attribute(meta.caller.module, :component_type)
quote generated: true do
component(
&(unquote(Macro.var(fun, __MODULE__)) / 1),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spacing here looks odd, but I assume that's not an actual issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's not an issue. I find that ugly too but that's the way the formatter handles it so there's not much we can do about it, I guess :)

@@ -0,0 +1,43 @@
defmodule Surface.Components.Component do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a 'props' prop to allow passing a map of props? My thinking is that otherwise users would get warnings about unknown props for this component

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They shouldn't get any warning but they can also use {...}. However, I'm not sure yet if that API is the best one. I'll create some tests for those cases and think how we can improve it.

@@ -0,0 +1,298 @@
defmodule Surface.Components.ComponentComponentTest do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
defmodule Surface.Components.ComponentComponentTest do
defmodule Surface.Components.DynamicComponentTest do

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Malian good catch! I was also thinking about changing the namespace from Surface.Components.Component to Surface.Components.Dynamic.Component as we still gonna have to implement a Surface.Components.Dynamic.LiveComponent in the near future. Since those built-in components are automatically aliased, I don't mind the larger name. WDYT?

Copy link
Collaborator

@miguel-s miguel-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work 👏

Can't add it to the review, but we might be able to remove the import Phoenix.HTML.Form from the Form component.

@@ -0,0 +1,43 @@
defmodule Surface.Components.Component do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going by the file name:

Suggested change
defmodule Surface.Components.Component do
defmodule Surface.Components.DynamicComponent do

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in the tests it seems you named it Component on purpose, it does read nicer 🤔

@msaraiva msaraiva merged commit 3a6a3a4 into master Sep 6, 2021
@msaraiva msaraiva deleted the ms-function-component branch September 6, 2021 21:55
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.

Support recursive components
4 participants