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

Updated Livewire to v3 #14831

Merged
merged 41 commits into from
Jun 27, 2024
Merged

Conversation

marcusmoore
Copy link
Collaborator

Description

This PR updates Livewire from v2 to v3.

I started with the automated upgrader before moving on to the manual changes that needed to be made.

Notables:

  • The standard namespace for Livewire in v3 is App\Livewire so our existing components have been moved there.
  • Alpine has been removed from the app's package.json since it is bundled with Livewire now. Since it is used in the new label engine I added @livewireScripts to that page. Any future pages that use Alpine but not Livewire will need to have that added.
  • The current state of the importer requires legacy_model_binding to be turned on but I would like to change that back to the default in the future.
  • There are some wire:model.lives that can probably have live removed but that's a small change that can happen in the future.

To test this make sure you:

  • composer install
  • nvm use
  • npm run dev

Please test thoroughly to double-check I didn't miss something.

The pages affected:

  • /account/api
  • /admin/oauth
  • /admin/slack
  • /categories/create and /categories/{id}/edit
  • /import
  • /models/create and /models/{id}/edit

Type of change

  • Library update

Copy link

what-the-diff bot commented Jun 5, 2024

PR Summary

  • Renaming of Key Files
    Numerous files were renamed within the project structure, mainly inside app/Http/Livewire/ folder to app/Livewire/.

  • Dependency Update
    The project dependency on livewire/livewire was upgraded to version ^3.0, as reflected in the composer.json file.

  • Configuration Modifications
    The config/livewire.php file underwent several changes which involve updates on class namespace, layout, placeholder settings, uploading properties, and other key configurations.

  • Javascript Event Binding Update
    The file resources/assets/js/snipeit.js saw an adjustment to how events are bound on the document object.

  • Layout Changes
    The files resources/views/layouts/basic.blade.php and resources/views/layouts/default.blade.php had changes revolving around the removal of some directives and movement of script loading.

  • View Component and Directive Adjustments
    Several resources/views/livewire/ files witnessed modifications - they mostly involve changes in properties and redefining how some elements are bound and interacted with.

  • Route Changes
    In routes/web.php, there were alterations to how the Importer class is accessed.

  • Test Class Name Update
    In the test file 'CategoryEditFormTest.php', the class name App\Http\Livewire\CategoryEditForm is changed to App\Livewire\CategoryEditForm.

  • Webpack Configuration Update
    Lastly, the file webpack.mix.js had the mix.combine directive removed.

These changes, overall, update the Livewire dependency, restructure the project a bit, streamline the configuration settings and improve user interface behavior.

@snipe snipe requested review from uberbrady June 5, 2024 19:14
|
*/

'render_on_redirect' => false,

'pagination_theme' => 'tailwind',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use tailwind tho?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have any components that are using pagination yet so I skipped that because it is possible to use bootstrap but it is the latest version so I would have to downgrade it to bootstrap v3's syntax.

I figured I'd skip that until we actually need pagination but maybe I should just include it so we don't have to worry about it in the future...

Gonna move this back to draft and include that...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right - we don't actually use Laravel pagination anywhere. You could probably just let it sit, since v8 will either be filament, OR will be just be more of our own API, which doesn't use UI pagination.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point...gonna leave this alone and just focus on the conflicts.

@snipe
Copy link
Owner

snipe commented Jun 12, 2024

@marcusmoore assuming we're still moving forward with v7 on Monday, do you want to pull this in for launch, or wait until any v7 bugs have shaken themselves out?

@snipe
Copy link
Owner

snipe commented Jun 12, 2024

(And that importer merge just caused a conflict, so... sigh.)

@marcusmoore marcusmoore marked this pull request as draft June 12, 2024 20:28
@marcusmoore
Copy link
Collaborator Author

@snipe I'll handle the swap to bootstrap and the conflicts. I'm ok with it being included in v7.0.0 as long as I get the changes done in time.

@marcusmoore
Copy link
Collaborator Author

@snipe conflicts resolved but let's hold off on including it in Monday's release until we get some other people pulling it down and testing it. I think it's good to go but would like other eyes on it to make sure I didn't miss something. It's affecting a good amount of functionality.

@snipe
Copy link
Owner

snipe commented Jun 12, 2024

@marcusmoore that's really reasonable. Will do, thank you.

@marcusmoore
Copy link
Collaborator Author

This is ready for review (and testing) now.

@marcusmoore marcusmoore marked this pull request as ready for review June 17, 2024 21:27
@marcusmoore
Copy link
Collaborator Author

@snipe just resolved some composer conflicts and this is ready for review/testing.

To test this branch:

  • composer install
  • nvm use (if you use nvm)
  • npm install
  • npm run dev
  • php artisan view:clear

These are the components/pages that are affected by this PR:

  • CategoryEditForm
    • Focus: Send email to user on checkin/checkout still automatically updates depending on text being present in Category EULA and/or Use the primary default EULA instead being checked
    • URLs:
      • /categories/create
      • /categories/1/edit
  • CustomFieldSetDefaultValuesForModel
    • Focus: Add default values checkbox toggles display of default field values
    • URLs:
      • /models/create
      • /models/1/edit
  • Importer
    • Focus: the whole thing...
    • URL: /import
  • OauthClients
    • Focus: create, edit, and delete clients
    • URL: /admin/oauth
  • PersonalAccessTokens
    • Focus: create, and delete tokens
    • URL: /account/api
  • SlackSettingsForm
    • URL: /admin/slack
  • The new label engine.
    • Focus: Field Definitions and Preview sections

@snipe snipe merged commit 54fb91c into snipe:develop Jun 27, 2024
9 checks passed
@snipe
Copy link
Owner

snipe commented Jun 27, 2024

Hm. In testing this locally, I'm getting https://snipe-it.test/models/1/edit when editing a model:

Call to a member function html() on string related to @livewire('custom-field-set-default-values-for-model',["model_id" => $item->id ?? $model_id ?? null ])

I'll see if I can get to the bottom of it before you come online. (I built assets and ran composer install, of course)

@snipe
Copy link
Owner

snipe commented Jun 27, 2024

Aaaaaand now I can't reproduce it. WTF.

@snipe
Copy link
Owner

snipe commented Jun 27, 2024

Seeing it pop up on the dev demo. Will try to manually clear those caches.

@marcusmoore marcusmoore deleted the chore/sc-23725/livewire3 branch June 27, 2024 16:56
@marcusmoore
Copy link
Collaborator Author

For any future reader: the fix is php artisan view:clear

@marcusmoore marcusmoore mentioned this pull request Jul 10, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants