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 lifecycle and mutation #4000

Merged
merged 2 commits into from
Feb 2, 2024

Conversation

Matsa59
Copy link
Contributor

@Matsa59 Matsa59 commented Jan 26, 2024

The goal is to support "add reorder" that happen in the same mutation.

Original PR: #3951 (that was revert because it introduced a bug)

@Matsa59 Matsa59 marked this pull request as draft January 26, 2024 19:41
@Matsa59
Copy link
Contributor Author

Matsa59 commented Jan 26, 2024

There is something weird, after my PR was merged (the original one), the code was rewrite for something incorrect 3196c6a

The PR was merged originally with this state c68aba0

That's why everything was broken (cf #3995).

I guess it's a merge error

@Matsa59 Matsa59 marked this pull request as ready for review January 26, 2024 19:56
@calebporzio
Copy link
Collaborator

Did my move to Sets not fix this?

@Matsa59
Copy link
Contributor Author

Matsa59 commented Jan 27, 2024

Did my move to Sets not fix this?

The first try was to update the mutation system. Then the code has been changed and to update the lifecycle system.

You've updated an old piece of code and I guess you force push it or something like that (so it eras the code ghat was merged)

@ekwoka
Copy link
Contributor

ekwoka commented Jan 28, 2024

@calebporzio The sets would only fix an issue of Alpine trying to initialize the element twice.

The concern in the original PR was that the element had both an add and a remove, but had never actually been initialized.

Alpine treated it as a "move" and not an "add", and thus just ignored it.

@ekwoka
Copy link
Contributor

ekwoka commented Jan 28, 2024

@Matsa59 Can you add a test to verify this works correctly?

Ideally first make it not work (without your changes) and then validate your changes against it.

@calebporzio
Copy link
Collaborator

Ah, I see. Thanks for the clarification @ekwoka. I'm slightly hesitant to pull this in for some reason - maybe I'd prefer more of that code to live in the mutation handler? Idk. It feels simple right now, and adding this check for an inited property doesn't feel right. Any opinions here? Any other ways to tackle this that we're not thinking of?

Also, yes tests would be great. Thanks.

The goal is to support "add reorder" that happen in the same mutation.
@Matsa59
Copy link
Contributor Author

Matsa59 commented Jan 30, 2024

@ekwoka test has been added, I used the same code as does not initialise components twice when contained in multiple mutations but I add/remove/add. This test failed on alpinejs main but works on my branch (as you asked to do)

Matsa59 referenced this pull request in phoenixframework/phoenix_live_view Jan 31, 2024
@Matsa59
Copy link
Contributor Author

Matsa59 commented Jan 31, 2024

Plus updating the lifecycle system may increase Alpine speed.

Currently (on main) the check if its a "move" operation, iterate on every deleted nodes for every inserted nodes. This could have a significant impact on performance.

@ekwoka
Copy link
Contributor

ekwoka commented Jan 31, 2024

Well, with the other recent change of using Set it should be much better already (but only guaranteed to be sublinear).

But yes, this should be a bit quicker.

@calebporzio
Copy link
Collaborator

Thanks @Matsa59 - in the future please checkout a non-main branch to PR back to this repo. Otherwise I can't commit to your branch.

I opened a new branch here: #4012

I'm going to merge it with a few tweaks after tests pass. Thanks!

@calebporzio calebporzio merged commit 4c2397c into alpinejs:main Feb 2, 2024
1 check passed
@Matsa59
Copy link
Contributor Author

Matsa59 commented Feb 2, 2024

Ok sure, I'll do another branch next time.

However on your tweaks you have some mistakes. You didn't rename every "_x_isInit"

@calebporzio 2f59ee2#r138214282

@calebporzio
Copy link
Collaborator

Haha good catch - yeah I see the failure and am working on it!

@joshhanley
Copy link
Collaborator

@PhiloNL is experiencing issues due to this PR - see this issue on his repo wire-elements/modal#423

Waiting to see if we can reproduce it in a smaller testcase.

@PhiloNL
Copy link
Contributor

PhiloNL commented Mar 8, 2024

It seems Alpine is no longer picking up any changes that happen after the initial render (using Livewire v2, in this example).

For example:

<?php

class Component extends Component
{
  public $ready = false;
}
<div x-data="{
        select() {
            console.log(this.$refs.resultGroups);
        }
    }" 

    wire:init="$set('ready', true)">
    
      @if($ready)
      <div x-ref="resultGroups">
          <div class="wep-spotlight-group">
              <ul x-ref="results">
                  <li @click="select" x-data>
                     Contacts
                  </li>
                  <li @click="select" x-data>
                     Billing
                  </li>
                  <li @click="select" x-data>
                     Docs
                  </li>
              </ul>
          </div>
      </div>
      @endif
      
</div>

Click any of the <li> and it will log undefined. I also had to add x-data to every <li> for the @click directive to work (which was not needed previously because it's a child element within an existing Alpine instance).

If you set ready to true by default, all works as expected. Once you set it to false and defer the loading of the <li> items it breaks.

@Matsa59
Copy link
Contributor Author

Matsa59 commented Mar 11, 2024

If you add x-data on the <div x-ref="resultGroups"> it should work.

@calebporzio I'm not sure reverting a PR that fixed another bug is a nice patch


edit: hum I guess the problem comes from x-ref that is not "refresh" when the dom is updated. Maybe we could find something on the x-ref init system.

Technically, the x-data form the parent is been init, so IMO it's not the problem of AlpineJS. Or the x-refs system should be calculate on exec and not on init

A fix that should work:

<div x-data="{
        resultGroups: nil,
        select() {
            console.log(this.resultGroups);
        }
    }" 

    wire:init="$set('ready', true)">
    
      @if($ready)
      <div x-data x-init="resultGroups = $el">
          <div class="wep-spotlight-group">
              <ul x-ref="results">
                  <li @click="select" x-data>
                     Contacts
                  </li>
                  <li @click="select" x-data>
                     Billing
                  </li>
                  <li @click="select" x-data>
                     Docs
                  </li>
              </ul>
          </div>
      </div>
      @endif
      
</div>

you could also use x-if or x-show and bind the var $ready to the js state

(or something like that, you get the idea, i didn't test it)

@PhiloNL
Copy link
Contributor

PhiloNL commented Mar 11, 2024

Thanks! But that would still be a breaking change for every Livewire v2 app out there that uses Alpine v3. So it would likely affect thousands of users. It would be great if no changes were required by the end-user.

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.

5 participants