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

bug: Module name discrepancy can lead to wrong expected view name #11

Open
thepeoplesbourgeois opened this issue Mar 25, 2022 · 0 comments · May be fixed by #12
Open

bug: Module name discrepancy can lead to wrong expected view name #11

thepeoplesbourgeois opened this issue Mar 25, 2022 · 0 comments · May be fixed by #12

Comments

@thepeoplesbourgeois
Copy link

tl;dr, I've got a pull request to make view_module an overridable function for apps that don't follow the de facto naming convention for LiveView modules. All unit tests pass. Thanks for building this 👍 🚀

First off, I want to express my gratitude to you for making this package. The controller paradigm is so helpful in keeping code well organized, especially (in my opinion) compared to the standard application structure of Phoenix Live View.

I'm incorporating live_controller into my Phoenix app now, and noticed that my app's structure surfaced a small usability issue for Phoenix apps with non-standard module namespaces. Specifically, I have made Live a namespace under my app's web namespace (as in PhoenixAppWeb.Live.{...}) to group together all of the common LiveView functionality.

As such, instead of e.g. PhoenixAppWeb.UserLive.Index and PhoenixAppWeb.UserLive.Show, or PhoenixAppWeb.UserLiveController modules, my app has PhoenixAppWeb.Live.User.Index, ...Web.Live.User.Show, and ...Web.Live.UserController modules. When determining its default view names, phoenix_live_controller anticipates that all live controllers have names that end with either Live or LiveController. Since mine don't, an exception was raised when I first added phoenix_live_controller, because ...Web.Live.UserController couldn't find a callable ...Web.Live.UserControllerView.render/2.

I forked this repository and added an overridable view_module function to the main module to fix this issue, as well as updated the String.replace/3 call to behave as expected when the controller module doesn't end with LiveController but rather just Controller. While implementing this, I noticed that live_controller's render function is also declared as overridable, and I considered just doing that in my app. Ultimately, I decided it would be better to add the overridable view_module/0 function, as overriding render would imply that some amount of work beyond changing the view's expected name would be necessary to properly render output for a controller action.

I've already got a pull request implementing this feature ready for you to review. It includes an update to the lockfile with the latest versions of all of live_controller's dependencies (your own ex_check among them) as well as a change to one of the unit tests to clear an unused variable warning from test runs. I hope you decide to merge my work.

Thank you again,
thepeoplesbourgeois

@thepeoplesbourgeois thepeoplesbourgeois linked a pull request Mar 25, 2022 that will close this issue
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

Successfully merging a pull request may close this issue.

1 participant