elixir-cldr / cldr_routes

Localised routes and path helpers for Phoenix applications
Other
14 stars 6 forks source link

Issue with :as option - atom or binary? #14

Closed kipcole9 closed 1 year ago

kipcole9 commented 1 year ago

We have the following LiveViews/folder structure:

In the router I specified the following routes:

localize do
      live "/#{locale}/article", MyAppWeb.ArticleLive.Index, :index
      live "/#{locale}/article/:id", MyAppWeb.ArticleLive.Show, :show
end

For the index paths the following routes and helper functions are generated (because of our naming convention of the LiveViews):

      index_de_path  GET   /de/article                                  MyAppWeb.ArticleLive.Index :index
      index_en_path  GET   /en/article                                  MyAppWeb.ArticleLive.Index :index

Because I don't want to rename our LiveViews I tried to use the as options of live/4 but the Cldr expects an binary value:

live "/#{locale}/article", MyAppWeb.ArticleLive.Index, :index, as: :article
** (ArgumentError) construction of binary failed: segment 1 of type 'binary': expected a binary but got: :article
    (ex_cldr_routes 0.6.2) lib/cldr/route.ex:622: Cldr.Route.helper_name/3
    (ex_cldr_routes 0.6.2) lib/cldr/route.ex:587: Cldr.Route.localise_helper/3
    (ex_cldr_routes 0.6.2) lib/cldr/route.ex:477: Cldr.Route.do_localize/4
    (ex_cldr_routes 0.6.2) expanding macro: Cldr.Route.localize/2

When I change the option to as: "article" the Phoenix LiveView router expects an atom:

** (ArgumentError) expected live :as to be an atom, got: "article"

    (phoenix_live_view 0.18.15) lib/phoenix_live_view/router.ex:400: anonymous fn/1 in Phoenix.LiveView.Router.validate_live_opts!/1
    (elixir 1.14.3) lib/enum.ex:975: Enum."-each/2-lists^foreach/1-0-"/2
    (phoenix_live_view 0.18.15) lib/phoenix_live_view/router.ex:387: Phoenix.LiveView.Router.validate_live_opts!/1
    (phoenix_live_view 0.18.15) lib/phoenix_live_view/router.ex:359: Phoenix.LiveView.Router.__live__/4
    lib/sourceboat_web/router.ex:38: (module)
    (elixir 1.14.3) lib/kernel/parallel_compiler.ex:340: anonymous fn/5 in Kernel.ParallelCompiler.spawn_workers/7

With the new localized verified routes feature this will no longer be an issue as the route helpers will no longer be needed, but I think it would also be nice to make this configurable. Or maybe we need to revise our naming conventions after all...

What do you think?

_Originally posted by @krns in https://github.com/elixir-cldr/cldr_routes/issues/12#issuecomment-1525052829_

kipcole9 commented 1 year ago

I think this is fixed now. Would you consider taking taking it for a spin? You'll need to configure as:

def deps do
  [
    ...
    {:ex_cldr_routes, GitHub: "elixir-cldr/cldr_routes", branch: "verified_routes"},
    ...
  ]
end
krns commented 1 year ago

I just tried your fix but when use the branch I get:

Because "your app" depends on "phoenix empty" which doesn't match any versions, version solving failed.
my-app-1       | ** (Mix) Hex dependency resolution failed

Never seen this error before.. maybe it's because we are still using Phoenix 1.6.16 but where is this "phoenix empty" coming from?

kipcole9 commented 1 year ago

The error isn't great for sure. But that branch is also dependent on Phoenix ~> 1.7 so its probably the cause. Let me backport that commit to the main branch which supports Phoenix ~> 1.6 and you should be ok to try from there. Will add a comment here when that's done.

kipcole9 commented 1 year ago

OK, I've back ported that change to the main branch so hopefully that will work for you (main doesn't include the verified routes support since that does require Phoenix ~> 1.7). Fingers crossed.

{:ex_cldr_routes, github: "elixir-cldr/cldr_routes"}
krns commented 1 year ago

Dams, same issue:

Running mix do deps.get
my-app-1       | * Updating ex_cldr_routes (https://github.com/elixir-cldr/cldr_routes.git - origin/main)
my-app-1       | Resolving Hex dependencies...
my-app-1       | Resolution completed in 0.02s
my-app-1       | Because "your app" depends on "phoenix empty" which doesn't match any versions, version solving failed.
my-app-1       | ** (Mix) Hex dependency resolution failed

I will do some research to see if the problem exists on my end.

kipcole9 commented 1 year ago

Are you running on the latest hex? Seems like a hex issue.

I'll publish a hex version now so you can run off that.

kipcole9 commented 1 year ago

I've published ex_cldr_routes version 0.6.4 with the following changelog entry:

Bug Fixes

Hope hex can resolve this for you!

krns commented 1 year ago

Thank you for the release. The :as option now works as expected with atoms, huge thanks!

However, I still have no idea why I cannot use branches of dependencies, but that doesn't seem to be a problem of the library - I'll do some research on that.

kipcole9 commented 1 year ago

Very happy to hear you're up and running. Perhaps try mix local.hex and see if there's an updated hex version available? I know the early in the hex 2.0 releases there were a few bugs (but I don't know enough to know if its related to your symptoms).

krns commented 1 year ago

Good guess, but I'm already on the latest version 2.0.6.