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

[2.x] Publish Livewire component classes and views as stubs #308

Closed
wants to merge 2 commits into from
Closed

[2.x] Publish Livewire component classes and views as stubs #308

wants to merge 2 commits into from

Conversation

calebporzio
Copy link
Contributor

@calebporzio calebporzio commented Oct 5, 2020

What this PR does:

Before: Only Livewire component views were published to the user's application.
After: Now upon running artisan jetstream:install livewire, both Livewire component views AND classes will be published.

Scroll to the end of this description for a glimpse of the new "published" file naming and structure.

Reasoning:

Before this PR, it was easy to to modify the Livewire component views, but not the classes. Users were only able to modify the code inside "Actions" used by components.

Sometimes, this is restricting and users being able to easily explore and edit the Livewire classes directly would be much simpler.

For example: Adding another input field to the "Update Team Name" form is currently difficult

Notes:

One side-benefit of this approach, is Jetstream no longer has to register the Livewire components. They now will get picked up by Livewire's auto-discovery feature. This aligns with the Livewire convention of where components live.

Although, I did test this PR briefly to make sure it worked, it is meant more as a reference because it contains opinions that likely differ from what @taylorotwell would do.

The opinions I'm referring to is mainly folder structure.

My philosophy when making these decisions was: How would I PERSONALLY structure these files if I were to create this functionality from scratch in an app of mine. As opposed to publishing files under a namespace like "Jetstream".

For that reason, feel free to close this and use it as a proof of concept or a starting point for making this change later on if at all.

Thank You ❤️

Huge thanks to both Taylor and the team that put so much thought, effort, and care into building and maintaining this package. I am extremely grateful for your generous time, money, and thought donations ❤️

File Structure Overview

app/
  Http/
    Livewire/
      NavigationDropdown.php
      Api/
        ApiTokenManager.php
      Profile/
        DeleteUserForm.php
        LogoutOtherBrowserSessionsForm.php
        TwoFactorAuthenticationForm.php
        UpdatePasswordForm.php
        UpdateProfileInformationForm.php
      Teams/
        CreateTeamForm.php
        DeleteTeamForm.php
        TeamMemberManager.php
        UpdateTeamNameForm.php

---

resources/
  views/
    dashboard.blade.php
    api/
      index.blade.php
    profile/
      show.blade.php
    teams/
      show.blade.php
      create.blade.php
    livewire/
      navigation-dropdown.blade.php
      api/
        api-token-manager.blade.php
      profile/
        delete-user-form.blade.php
        logout-other-browser-sessions-form.blade.php
        two-factor-authentication-form.blade.php
        update-password-form.blade.php
        update-profile-information-form.blade.php
      teams/
        create-team-form.blade.php
        delete-team-form.blade.php
        team-member-manager.blade.php
        update-team-name.blade.php

@calebporzio calebporzio changed the title Publish Livewire component classes and views as stubs [2.x] Publish Livewire component classes and views as stubs Oct 5, 2020
@m1guelpf
Copy link
Contributor

m1guelpf commented Oct 6, 2020

Isn't this similar to publishing the controllers for the Inertia driver (something that Taylor has already said doesn't wanna do)?

@taylorotwell
Copy link
Member

Before merging this I would like to have a more concrete discussion about what would actually be customized and could we solve it another way.

The team name thing, which I discussed with Caleb privately, I'm not certain about. My hunch would be to just add a new panel yourself to the team/show page to manage whatever other settings you want and leave the team name portion alone.

@devingray
Copy link

Personally when working with the Livewire version of Jetstream this will provide a lot of value for me. Although the current set up is not that difficult to customize.

@driesvints
Copy link
Member

@calebporzio one conflict here.

@taylorotwell
Copy link
Member

@devingray can you give more details on how you would use this specifically?

@devingray
Copy link

devingray commented Oct 8, 2020

@taylorotwell first of all, I am in no way saying Jetstream is set up incorrectly I can fully understand why it is the way it is, also it will only grow over time.

With that said

I have a number of use cases that I can think of that I can do in inertia using JS, but in Livewire it is a bit more tricky to achieve.

First use case (Events and Jobs attached to specific actions)

As an admin, I want to be notified when a user disables their two factor auth.

With Inertia and Vue I could simply add an additional call in the Vue.js

       disableTwoFactorAuthentication() {
                this.disabling = true

                this.$inertia.delete('/user/two-factor-authentication', {
                    preserveScroll: true,
                }).then(() => {
                    this.disabling = false
                    // do something here to notify admin
                })
            },

With Livewire however this is different as I would need to fire an event or dispatch a job (or even call an action) to get this done. Currently any work around would need to be done by extending the component and overriding it

Livewire::component('profile.two-factor-authentication-form', MyCustomClass::class);

The same could be said for similar use cases like
Sending an email when a password is changed
Sending an email when Api Tokens are deleted


Second use case is for UI and UX

Livewire offers a great way to use browser events, $this->dispachBrowserEvent() and I can use alpine to build tools like toasts, confirms etc, currently to get to add these small niceties is a manual effort to override the classes.

--

Please don't get me wrong, I do not blindly believe that we should just publish all classes and allow the user to do as they please as a one and done, the way it is set up makes a lot of sense for longevity and updates etc meaning I will be able to get the best of Jetstream in future versions with a much simpler update approach. That is a great selling point for me.

So there are pros and cons as always.

Currently I have managed to get everything I have needed working with some form of work around, but having the option to publish these things for livewire would have made my learning curve a bit easier. Although it wasn't very hard.

Perhaps to add more class binding methods that can be overridden for every use case is the better way to achieve all of this

EG

    public static function deleteTwoFactorAuthUsing(string $class)
    {
        return app()->singleton(DeletesAuth::class, $class);
    }

Something that userland can override for all functions in Jetstream which is consistent with the rest of the package.

This could also be a good option

#313 (comment)

@dukenst2006
Copy link

dukenst2006 commented Oct 9, 2020

I think this PR could be useful, i want to dispatch event to alert the user after any action, with livewire generally i used javascript toastr library.
// show alert
$this->dispatchBrowserEvent( 'alert', ['type' => 'success', 'message' => 'User saved']);
I can't do that actually in the current jetsream version cause components not published.
Thanks for Jetstream and all the hard work.

@joelbutcher
Copy link
Contributor

@calebporzio Just a side note, would it not make more sense to use views/Jetstream as the resource directory nameinstead of 'livewire'?

I think this would make a better convention for this package and keep consistency across both stacks as this is how the InertiaJS components are grouped (js/Jetstream) - just a thought

@Matthijs-Huisman
Copy link

I personally think it will make things easier te work with when developing new apps, something Laravel is always been good at. It takes some time to fully wrap your head around the working of Jetsteam when you just get started with it, this might also make that part easier. Downside is the future upgrades on the published files would be manual work.

@relaypilot
Copy link

I agree with the above comments, publishing the components would make it easier for people like me that are not too confident in overriding classes themselves or willing to venture in the vendor folder too much.

Anything that can allow people to have more flexibility over the predefined logic (views, components, controllers, route names...) is a good thing to have in my opinion.

Even if at the moment it is not that hard to achieve manually, it still feels like extra work that could potentially be dealt with with an artisan command?

Anyway, great product just the way it is, I just wanted to share my thought on this.

@taylorotwell
Copy link
Member

Personally I'm going to hold off on this for the time being. Every use case so far presented can be solved by simply PRing a few new events to Jetstream itself.

@relaypilot if you want to customize route names, components, controllers, etc. just use Fortify and build your own frontend IMO.

@OscarKolsrud
Copy link
Contributor

@taylorotwell Is this still a no go? By publishing it allows Jetstream to be a more flexible scaffolding thus allowing use of Jetstream even when you plan to do something a bit custom.

@heychazza
Copy link

Hey @taylorotwell, please can you reconsider this option if possible. It makes it impossible to add extra photo options because these are not present and unable to publish them.

@calebporzio will this PR work for the latest Jetstream version as of right now? As I may just use your fork additions for the time being.

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 this pull request may close these issues.

None yet