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

Add the ability to strip data-test- attributes from HEEX output #3649

Conversation

andrewtimberlake
Copy link

This suggested change is inspired by ember-test-selectors
To make tests more robust, you can use test specific attributes like data-test-user-id={@user.id} for easier element lookup and identification without a reliance on the specifics of the HTML markup, but this is test specific code that doesn’t belong in production.
This change adds a compilation flag to strip all test attributes which can be placed in the config/prod.exs file

@SteffenDE
Copy link
Collaborator

I did something similar which doesn’t require any changes to LV in a work project:

I defined a function in my core components:

if Mix.env() in [:test, :e2e] do
  def testid(id), do: id
else
  def testid(_id), do: nil
end

using it like this:

~H"""
<div data-test-id={testid("foo")}>…</div>
"""

@andrewtimberlake
Copy link
Author

That works.
It’s a little verbose for things like <div data-test-header> which has to be <div data-test-header={testid("")}>
It would also be nice to have an easy way to write less brittle tests baked in.

@josevalim
Copy link
Member

@SteffenDE your version still does runtime work. The version above does everything at compile-time, although you have to be very careful, cause you may get unused variable warnings if you do something like:

def foo(%{id: id}) do
  ~H"""
  <div data-test-foo={id}>
    ...
  </div>
  """
end

And the id is only used there.

So I can see benefits with this feture, although I would make it a bit more generic and allow you to pass the attribute prefix as configuration.

@SteffenDE
Copy link
Collaborator

I know that my version still has a runtime overhead. I didn’t benchmark it, but I don’t think it is something you’d need to worry about. That being said I also see the benefit here, so I support this change.

I do wonder though what would be necessary to support macros in HEEx expression that we could expand at compile time and then directly strip falsy attributes. Does this make sense?

@josevalim
Copy link
Member

Actually, a macro is perfect:

if Mix.env() in [:test, :e2e] do
  defmacro testid(id), do: id
else
  defmacro testid(_id), do: nil
end

And we can be made sure to optimize it in the future. So, in my opinion, this can be fully tackled in user-land with more transparency. Thank you for the discussion @andrewtimberlake but for now I don't see a reason to go ahead with this PR, which keeps the removing of the attribute hidden very far away in a config file. :)

@andrewtimberlake
Copy link
Author

Thanks for the consideration.

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.

3 participants