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

user configurable self closing tags #417

Merged
merged 2 commits into from
Sep 6, 2022

Conversation

inoas
Copy link
Contributor

@inoas inoas commented Aug 24, 2022

Fixes issue 405.

Notice: Instead of just appending to the tag list, I have opted to expose the internal default tag list and replace it.

The reason for this is that in some specific xml dialects people would want to write they could replace <img/> with <img>...<img/> and floki would not be able to support that.

The trade-off here in terms of DX is that if you want to keep the standard html self closing tags you cannot just do:

config :floki, :self_closing_tags, ["download"]

but you need to do

config :floki, :self_closing_tags, ["tags", "you", "want"]

I am willing to document this and I hope that this solution passes your checks.

Please let me know if the way I have added unit tests is the the right approach for you.

p.s.: I have tested the feature in my small real world application where I am using a custom XMl dialect to allow a subset of html and extend it by some specific tags of which some are self-closing:

# In my userland application `config/config.exs`
config :floki, :self_closing_tags, [
  "article",
  "br",
  "download",
  "hr",
  "media",
  "page",
  "shy",
  "space",
  "wbr"
]

@inoas
Copy link
Contributor Author

inoas commented Aug 24, 2022

Further notes:

  1. Once this is GTM, I can squash this PR's commits to remove the noise.
  2. I have opted to let Floki.RawHTML.default_self_closing_tags and Floki.RawHTML.self_closing_tags be public so that user land applications can list and/or document self closing tags programatically.
  3. I tried this:
    config :floki, :self_closing_tags, Floki.RawHTML.default_self_closing_tags() ++ ["br"]
    ... in config/config.exs but at that stage the module is not compiled/available yet. I am open to suggestions to make it easier to extend the list of self closing tags instead of overwriting them.

@inoas
Copy link
Contributor Author

inoas commented Aug 24, 2022

I have added menuitem because we also support command and keygen, all 3 are obsolete nowadays but that doesn't mean that they don't exist in the wild, so by default I'd include all 3.

@philss
Copy link
Owner

philss commented Aug 24, 2022

@inoas I was thinking maybe we should use Application.compile_env/3 because it would be faster and the code could stay almost the same as before (using module tag). The only drawback would be testing, which is not possible since it's compiled I think. But I guess this is fine. WDYT?

@philss
Copy link
Owner

philss commented Aug 24, 2022

@inoas and BTW, thanks for the PR! 💜

@inoas
Copy link
Contributor Author

inoas commented Aug 24, 2022

@inoas I was thinking maybe we should use Application.compile_env/3 because it would be faster and the code could stay almost the same as before (using module tag). The only drawback would be testing, which is not possible since it's compiled I think. But I guess this is fine. WDYT?

Do you mean like here? https://github.com/philss/floki/pull/417/files#diff-477200ed1da650829519cb2d3f87fdd9a8b3d5f8d786993494dbbeceb795445eR27

I mean sure I can switch this around and remove the tests.
But I don't think the changes I did will really impact performance, do you have some tools to measure?

What are you concerned about: the get_env call for every tag or the move from guards to cases because I don't think that the latter will have a huge impact (but the former may).

If there are ways to do this the right way (tested + fast) via macros and you have an idea, just point me into the direction and I will see what I can do about it.

@philss
Copy link
Owner

philss commented Aug 24, 2022

@inoas let's keep it simple and go with your current version. We can iterate later and see if there is any gain to use the compile_env.

@inoas inoas force-pushed the master+configurable-self-closing-tags branch from 80c9df0 to ad88c59 Compare September 3, 2022 09:39
@inoas
Copy link
Contributor Author

inoas commented Sep 4, 2022

please trigger ci

Copy link
Owner

@philss philss left a comment

Choose a reason for hiding this comment

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

Thank you! 💜

I sent a suggestion, but I'm going to commit it already.

lib/floki/raw_html.ex Outdated Show resolved Hide resolved
@philss philss merged commit 4151c84 into philss:master Sep 6, 2022
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.

Suggestion self_closing_tags
2 participants