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

Removing plug dependency from this project #210

Closed
CrowdHailer opened this issue Apr 21, 2018 · 8 comments
Closed

Removing plug dependency from this project #210

CrowdHailer opened this issue Apr 21, 2018 · 8 comments

Comments

@CrowdHailer
Copy link

Plug seams a very large dependency for this project. Mostly it looks like it is included to handle HTML escaping. Without much duplication at all that could be copied in or a package like eex_html could be created for it or such functionality could be added to eex itself.

I'm aware that if using this in phoenix then plug is already a dependency.
I am using this in a project and plug is by far the largest dependency brought in and it has noticeably changed compile times.

I assume this has been extracted from the phoenix codebase because it is a useful component in it's own right. I would like to use it in a project without plug, and I can think of cases beyond my, such as:

  • A service that sends HTML emails might use this library for templating emails
  • You could use this and Elixirscript to make clientside applications
@josevalim
Copy link
Member

Even if we remove the HTML handling bits, the whole link and form functionality depends on Plug.

Also, if in the future we believe relying more on Plug would lead to a better experience for Phoenix developers, then we would like to do that too.

One could suggest for us to break the part that depends on Plug and the one that does not in two separate projects but then the usability suffers a lot. Phoenix docs are slightly tricky to follow/search, because you need to travel between ecto, phoenix and phoenix_html. Adding a separate package would complicate it even further.

The other suggestion I can think of is to extract the eex_engine + html_safety functions to a separate project. Those two functionalities are pretty much the foundation of this library and doing so would allow others to leverage the same foundation. But I am not sure if this would actually be useful to others.

@josevalim
Copy link
Member

I will go ahead and close this issue since we don't plan to remove the Plug dependency. We can discuss stripping the eex engine + HTML safety stuff though if folks believe that's desirable and are willing to do the work on here and on Phoenix.

@CrowdHailer
Copy link
Author

I have now pulled out what I think would constitute a EEx.HTMLEngine and a general HTML.Safe protocol. https://github.com/CrowdHailer/raxx/blob/master/lib/eex/html_engine.ex

Would there be any interest on adding HTMLEngine to the EEx project. then it would not increase the number of dependencies. I also think it is a fairly well isolated pieces of functionality so would not be much in the way of bloat on core.

Otherwise I am thinking of starting a library called eex_html but that is likely to be small and have all the disadvantages of one more small dependency.

@josevalim
Copy link
Member

I have debated this for a while. To me the issue is that HTMLEngine depends on the protocol and I would like to keep the number of protocols in Elixir to a minimum. There is an implicit requirement that any new datatype should implement (or at least worry about) all protocols in Elixir and adding new protocols increase the surface area.

Plus it works completely fine outside of Elixir, so it doesn't make a strong case for adding it to the language itself.

@josevalim
Copy link
Member

Btw, if you used the code from this repository, please don't forget to include the license and copyright notice, as those are the conditions for modifying and distributing code from this repo.

@CrowdHailer
Copy link
Author

Plus it works completely fine outside of Elixir, so it doesn't make a strong case for adding it to the language itself.

Agreed. The strongest reason I had for adding it was avoiding a proliferation of small libraries. As you mentioned even if eex_html existed it is unlikely to get used in this project.

There is an implicit requirement that any new datatype should implement (or at least worry about) all protocols in Elixir

The fallback to using String.Chars which was then auto escaped seemed a nice solution to this, or at least was a plausable middle ground.

Re the license, only thing I have copied is the html escaping, which is from plug

@josevalim
Copy link
Member

As you mentioned even if eex_html existed it is unlikely to get used in this project.

Yeah. But you should probably consider releasing as a package anyway. We would use it if it was not for backwards compatibility. Also, you should consider not defining things inside EEx namespace. If by any chance EEx decide to add something related to HTML in the future, it means you will either break EEx or EEx will break your package. So using EExHTML or similar may be a better option. Or call it HTMLSafe or something. Dunno.

Re the license, only thing I have copied is the html escaping, which is from plug

Oh, beautiful. I didn't check the code, so I just mentioned it. :)

@CrowdHailer
Copy link
Author

Moved the discussion around possibly adding to core (however unlikely) here.

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

No branches or pull requests

2 participants