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

feat: update to Phoenix HTML 4.1, bump dependencies #4277

Merged
merged 17 commits into from
Oct 27, 2024

Conversation

JakobLichterfeld
Copy link
Collaborator

No description provided.

@JakobLichterfeld JakobLichterfeld added dependencies Pull requests that update a dependency file area:teslamate Related to TeslaMate core labels Oct 19, 2024
@JakobLichterfeld JakobLichterfeld mentioned this pull request Oct 19, 2024
1 task
Copy link

netlify bot commented Oct 19, 2024

Deploy Preview for teslamate ready!

Name Link
🔨 Latest commit 91f23a3
🔍 Latest deploy log https://app.netlify.com/sites/teslamate/deploys/671de659c9e73d00080dc5d0
😎 Deploy Preview https://deploy-preview-4277--teslamate.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@JakobLichterfeld
Copy link
Collaborator Author

Thanks @sdwalker for your changes!

@brianmay
Copy link
Collaborator

Tests failing?

@JakobLichterfeld
Copy link
Collaborator Author

Tests failing?

Yeah, I assume this is the cause:

warning: defining a Gettext backend by calling

    use Gettext, otp_app: ...

is deprecated. To define a backend, call:

    use Gettext.Backend, otp_app: :my_app

Then, instead of importing your backend, call this in your module:

    use Gettext, backend: MyApp.Gettext

  lib/teslamate_web/gettext.ex:23: TeslaMateWeb.Gettext (module)

@JakobLichterfeld JakobLichterfeld marked this pull request as draft October 20, 2024 08:35
@sdwalker
Copy link
Contributor

The mix files need cleaned to only have the phoenix changes and not include the rest of the unrelated package updates

The gettext warnings are simple fixes when updating gettext

The dependencies that aren't keeping up with package releases (websockex, timex) are another source of warnings
Bulma can be updated to 1.0.2 if you ignore the slew of deprecation warnings but their devs are keeping up

@JakobLichterfeld JakobLichterfeld changed the title feat: update to Phoenix HTML 4.1 feat: update to Phoenix HTML 4.1, bump dependencies Oct 21, 2024
@JakobLichterfeld
Copy link
Collaborator Author

Yeah, I assume this is the cause:

warning: defining a Gettext backend by calling

    use Gettext, otp_app: ...

is deprecated. To define a backend, call:

    use Gettext.Backend, otp_app: :my_app

Then, instead of importing your backend, call this in your module:

    use Gettext, backend: MyApp.Gettext

  lib/teslamate_web/gettext.ex:23: TeslaMateWeb.Gettext (module)

I fixed it with the help of https://www.yellowduck.be/posts/fixing-the-gettext-warning-in-phoenix

@brianmay
Copy link
Collaborator

brianmay commented Oct 21, 2024

It looks like this function call

|> Cloak.Vault.read_config()
is returning a tuple, and we do not expect a tuple.

This puzzles me though, I don't see any changes here.

Oh, I think this change upgraded Clock, so we got this breaking change: danielberkompas/cloak@267077e.

Which is typical fashion wasn't documented as a breaking change :-(

https://github.com/danielberkompas/cloak/blob/master/CHANGELOG.md

@brianmay
Copy link
Collaborator

If I am reading the stack trace correctly, this is returning an error tuple:

{:ok, attrs} = @geocoder.details(addresses, lang)

Which seems to indicate we are getting here somehow:

throw({:error, :boom})

@sdwalker
Copy link
Contributor

Yeah, I assume this is the cause:

warning: defining a Gettext backend by calling

    use Gettext, otp_app: ...

is deprecated. To define a backend, call:

    use Gettext.Backend, otp_app: :my_app

Then, instead of importing your backend, call this in your module:

    use Gettext, backend: MyApp.Gettext

  lib/teslamate_web/gettext.ex:23: TeslaMateWeb.Gettext (module)

I fixed it with the help of https://www.yellowduck.be/posts/fixing-the-gettext-warning-in-phoenix

They match the local changes I've been using

@JakobLichterfeld
Copy link
Collaborator Author

Oh, I think this change upgraded Clock, so we got this breaking change: danielberkompas/cloak@267077e.

Which is typical fashion wasn't documented as a breaking change :-(

https://github.com/danielberkompas/cloak/blob/master/CHANGELOG.md

Wow, nice catch. Ty!
I downgraded Credo and it looks much better now.

@JakobLichterfeld JakobLichterfeld marked this pull request as ready for review October 22, 2024 08:15
@JakobLichterfeld
Copy link
Collaborator Author

JakobLichterfeld commented Oct 24, 2024

2 Test failures which needs to be fixed for the upcoming release (this is the blocker):

  1) test language shows error (TeslaMateWeb.SettingsLiveTest)
Error:      test/teslamate_web/live/settings_test.exs:189
     ** (FunctionClauseError) no function clause matching in Floki.find/2

     The following arguments were given to Floki.find/2:

         # 1
         nil

         # 2
         "p.help"

     Attempted function clauses (showing 2 out of 2):

         def find(html, selector) when -is_binary(html)-
         def find(html_tree_as_tuple, selector) when -is_list(html_tree_as_tuple)- or -is_binary(html_tree_as_tuple)- or -tuple_size(html_tree_as_tuple) == 3- or -tuple_size(html_tree_as_tuple) == 2- and (-elem(html_tree_as_tuple, 0) === :pi- or -elem(html_tree_as_tuple, 0) === :comment-) or -tuple_size(html_tree_as_tuple) == 4- and -elem(html_tree_as_tuple, 0) == :doctype-

     code: TestHelper.eventually(fn ->
     stacktrace:
       (floki 0.36.2) lib/floki.ex:277: Floki.find/2
       test/teslamate_web/live/settings_test.exs:231: anonymous fn/2 in TeslaMateWeb.SettingsLiveTest."test language shows error"/1
       (teslamate 1.30.2-dev) test/support/test_helper.ex:7: TestHelper.eventually/3
       test/teslamate_web/live/settings_test.exs:210: (test)

................................

  2) test Edit validates changes when editing of a geo-fence (TeslaMateWeb.GeoFenceLiveTest)
Error:      test/teslamate_web/live/geofence_live_test.exs:112
     Assertion with == failed
     code:  assert error_html ==
              "<span class=\"help is-danger pl-15\" phx-feedback-for=\"geo_fence_#{kind}\">can't be blank</span>"
     left:  "<span class=\"help is-danger pl-15\" phx-feedback-for=\"geo_fence[name]\">can't be blank</span>"
     right: "<span class=\"help is-danger pl-15\" phx-feedback-for=\"geo_fence_name\">can't be blank</span>"
     stacktrace:
       (elixir 1.17.2) lib/enum.ex:1703: Enum."-map/2-lists^map/1-1-"/2
       test/teslamate_web/live/geofence_live_test.exs:150: (test)

@JakobLichterfeld
Copy link
Collaborator Author

JakobLichterfeld commented Oct 24, 2024

first is related to:

|> Floki.find("p.help")

second is related to:

@brianmay
Copy link
Collaborator

I think there have been some minor changes in how field names are generated:

<div class="field is-expanded">
        <div class="control">
<input class="input" id="geo_fence_name" name="geo_fence[name]" placeholder="Name" type="text" value="">
        </div>

          <p class="help is-danger"><span class="help is-danger pl-15" phx-feedback-for="geo_fence[name]">can't be blank</span></p>

      </div>

What confused me for a while is that the phx-feedback-for points to the name not the id field, from the docs:

The phx-feedback-for annotation specifies the name (or id, for backwards compatibility) of the input it belongs to. Failing to add the phx-feedback-for attribute will result in displaying error messages for form fields that the user has not changed yet (e.g. required fields further down on the page).

https://hexdocs.pm/phoenix_live_view/form-bindings.html

That explains the 2nd error at least.

@sdwalker
Copy link
Contributor

first is related to:

|> Floki.find("p.help")

Breaks upgrading floki 0.35.2 => 0.35.3

@sdwalker
Copy link
Contributor

0.19.0 (2023-05-29)
Backwards incompatible changes

Drop support for passing an id to the phx-feedback-for attribute. An input name must be passed instead.

@JakobLichterfeld
Copy link
Collaborator Author

Breaks upgrading floki 0.35.2 => 0.35.3

We love it, when there are breaking changes in patch releases... Even if it's not mentioned in the changelog: https://hexdocs.pm/floki/changelog.html#0-35-3-2024-01-25

@JakobLichterfeld
Copy link
Collaborator Author

JakobLichterfeld commented Oct 25, 2024

We love it, when there are breaking changes in patch releases... Even if it's not mentioned in the changelog: https://hexdocs.pm/floki/changelog.html#0-35-3-2024-01-25

Downgrading floki is not an option, as phoenix_live_view depend on 0.36+

@JakobLichterfeld
Copy link
Collaborator Author

JakobLichterfeld commented Oct 26, 2024

I got rid of the second error, but quite sure this can be handled better. Pushes are welcome

Imo only the test was wrong, as we set phx_feedback_for correct with name instead of id:

phx_feedback_for: input_name(form, field)

@sdwalker
Copy link
Contributor

sdwalker commented Oct 26, 2024

Test passes if the div match is removed

|> Enum.find(
&match?(
{"div", _,
[
{_, _,
[
{_, _,
[{_, _, [{"select", [{"id", "global_settings_language"}, _], _}]}]},
_
]}
]},
&1
)
)

@JakobLichterfeld JakobLichterfeld merged commit 2a922e3 into master Oct 27, 2024
20 checks passed
@JakobLichterfeld JakobLichterfeld deleted the bump-to-Phoenix-HTML-4-1 branch October 27, 2024 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:teslamate Related to TeslaMate core dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants