-
Notifications
You must be signed in to change notification settings - Fork 66
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 Phoenix 1.4 #64
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much @scarfacedeb!! This is really great. I'm going to give @Rakoth a chance to take a peek before I merge 😁
Any news on this? Phoenix 1.4 was since released (would be great it @scarfacedeb could update the PR accordingly). Would be great to get this merged and released, soon. If there's any help we can provide please let me know. |
Phoenix.* tasks has been removed in Phoenix 1.4.
9ffb2e8
to
dd3ce4a
Compare
@mschae @doomspork I updated I think it should be ready to merge. |
@doomspork can you merge this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one, @scarfacedeb !
[ | ||
{:phoenix, "~> 1.4"}, | ||
{:phoenix_html, "~> 2.10"}, | ||
{:jason, "~> 1.0", optional: true}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need jason in dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get rid of the following Phoenix warning:
~/t/phoenix_slime ❯❯❯ mix test
Compiling 5 files (.ex)
Generated phoenix_slime app
warning: failed to load Poison for Phoenix JSON encoding
(module Poison is not available).
Ensure Poison exists in your deps in mix.exs,
and you have configured Phoenix to use it for JSON encoding by
verifying the following exists in your config/config.exs:
config :phoenix, :json_library, Poison
(phoenix) lib/phoenix.ex:40: Phoenix.start/2
(kernel) application_master.erl:277: :application_master.start_it_old/4
...........
Finished in 1.1 seconds
11 tests, 0 failures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be only for test env then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe optional: true
is the way to go there...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about another issue with the current approach:
Now we have implicit configuration (config :phoenix, :json_library, Jason
) that sets config value that is unrelated to phoenix_slime
and may break someone else's code.
Therefore I think it's better to move the dependency into :test
env and config into config/test.exs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config is not a part of a package, don't worry
@doomspork I think, it is good to merge! |
@doomspork do the needed please :) |
Sorry about that @scarfacedeb! I didn't realize @Rakoth didn't have permissions (I fixed that). We're always looking for more contributors if you're interesting in helping maintain the Phoenix packages (and create one for LiveView). I'm not currently using Phoenix for many of my projects at work so keeping up with the developments to stay ahead of these packages is tough. |
Any chance of getting a new release with this merge as well please? Thanks for all your work peeps! ❤️ |
@doomspork Sure, I'll be glad to help! I'm building API phoenix apps for the most part, but I have a couple of personal projects with slim templates too. |
@navinpeiris done! Sorry for the delays, lots on my plate these days 🙁 |
completely understand @doomspork, thanks so much! ❤️ |
I updated deps to support Phoenix 1.4.0-rc and dropped Cowboy, because I don't think it's needed here.
Also I deleted
phoenix.*
tasks, because phoenix 1.4 removed them too.