Closed Hermanlangner closed 4 months ago
I am open to 4.x, but we need to support 3.x for some time, I am not sure about the reason for the changes in mix.lock file, did you run mix deps.update?. Anyhow see if you can get rid of the warnings and not make changes to mix.lock file.
I am open to 4.x, but we need to support 3.x for some time, I am not sure about the reason for the changes in mix.lock file, did you run mix deps.update?. Anyhow see if you can get rid of the warnings and not make changes to mix.lock file.
I did not consider backwards compatibility, so I did a deps.update all. I'll revert that but and remove the warnings. I will still need to add the package for backwards compability though
@ananthakumaran I've tried keeping changes to the minimum, for the html I did use mix format since I couldn't get the indentation looking consistently, there's nothing that changed in format that's not out of the box elixir formatting. Apologies for the few extra line changes.
For the tests, they were very brittle in expecting changes on the same line, I expanded and red green tested to make sure they were correct, they'll now not be coupled to the html structure as they were before. In the future it's probably even better to be more explicit than a regex selector.
I've tried pointing out the changes I made to make it easier to review.
I've kept as much to the bare minimum as possible.
I had trouble getting my local back to 12 and 13 so I've not been able to test them, as even the vms aren't great anymore through asdf.
I feel the ci runners should be updated for newer versions of elixir, but this is entirely your call.
Phoenix html 3.3 is fully supported thanks to the helpers package, but I don't know about older versions of elixir. I'm happy to make small adjustments to cater for it.
I do feel that package versioning exists for this reason of supporting the newer and dropping the older, however this is ultimately your package, and you decide these things.
If we can't reach some point of agreement I'll be forced to fork it, as my time is limited, but I'm doing this to try and give back to a library that's been incredibly useful for us.
I had trouble getting my local back to 12 and 13 so I've not been able to test them, as even the vms aren't great anymore through asdf.
I feel the ci runners should be updated for newer versions of elixir, but this is entirely your call.
agree, I am ok with dropping 1.12 (which seems to be missing support for Keyword.validate!), adding newer versions can be handled as a separate PR
I do feel that package versioning exists for this reason of supporting the newer and dropping the older, however this is ultimately your package, and you decide these things.
Compared to other libraries I maintain, exq_ui has to be constantly updated thanks to phoenix and phoenix live view changes :). I try to support last 2 versions. phoenix_html 4.x is brand new (December 2023) and I can't drop support for 3.x as we have projects using exq_ui and depend on 3.x
Phoenix html 3.3 is fully supported thanks to the helpers package,
if this is true, I don't have much issues. The changes looks good so far, the only request I have is keep running CI with phoenix HTML 3.x (which got switch to 4.x)
If we can't reach some point of agreement I'll be forced to fork it, as my time is limited, but I'm doing this to try and give back to a library that's been incredibly useful for us.
Fully understand, don't worry too much. If you run short on time feel free to use a fork for some time. Eventually, I or someone else will get the required changes done.
@ananthakumaran I tried bumping down the dependencies, unfortunately it looks like phoenix_html_helpers
provides backwards compatibility to phoenix_html 3.3 but it depends on phoenix_html 4.
It will unfortunately not be possible to update while having 3.x as a dependency, which is a bummer, I was hoping to get best of both worlds.
I fully understand the supporting 2+ versions, and hopefully this work does become useful when it becomes time to make the switch!
If this is picked up in the future, I had still missed some manual testing before I was going to merge, I wrongly copy pasted the routes in the navcomponent. Don't let that slip through
Thanks for the PR. I will come back to this in a month or two and would surely be useful.
@Hermanlangner would you be able to remove the package name change related commits. I want to get this merged. Let me know if you are busy, I can cherry-pick other commits and create a new PR
Master supports phoenix html 4
On our App we use exq_ui, and I was going on a dependency update mission.
Exq_ui is using
phoenix_html: 3.x
butphoenix_html: 4+
has compability issues with it. Namely inuse Phoenix.HTML
As in the error message hereI did exactly as the prompt suggested, which should at least add compatibility for a while. It does generate warnings, but that would involve changing the helpers and I'm not sure what the author would prefer to change it to.
This PR is simply enough to bump dependencies