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

Allow debouncing/throttling x-model when using x-modelable #4186

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Spitfire972
Copy link

Solves
#2812

Problem

When using x-model.debounce with x-modelable the outer values will not be debounced. I belive this to be a bug as the documentation says the following about x-modelable:

It's useful for abstracting away Alpine components into backend templates and exposing state to the outside through x-model as if it were a native input.

Solution

I have not modified the el._x_model.set function as you would expect this to ignore the throttle/debounce (the same way it does for normal x-models). Instead I have added an extra method (setWithModifiers) which respects debounce and throttle modifiers. I have then changed the outerSet of x-modelable to use setWithModifiers (still allowing the inner value to update live).

Tests

I have included one test for debounce and throttle respectively (as they are implemented slightly seperate from each other, I thought this reasonable).

@ekwoka
Copy link
Contributor

ekwoka commented May 1, 2024

For some clarity, the outer value means the theoretical state side (like a normal components data), while the inner value means the x-modelable (like the inputs value)?

@Spitfire972
Copy link
Author

For some clarity, the outer value means the theoretical state side (like a normal components data), while the inner value means the x-modelable (like the inputs value)?

Yes, sorry. outer and innner are the terms used in the codebase. If we look at this example:

<div x-data="{ number: 5 }">
    <div x-data="{ count: 0 }" x-modelable="count" x-model.debounce="number">
        <button @click="count++">Increment</button>
    </div>
 
    Number: <span x-text="number"></span>
</div>

then outer is number and inner is count. In this example if number was set directly then count would update immedietly, and if count was set then number would update whilst respecting debounce.

This behaviour would be consistent with how a normal x-model works with input elements.

@Spitfire972
Copy link
Author

The other option I have just throught of is to emit "input" events from the modelable. I have no Idea if this would work, but it would allow the x-model code to remain unchanged. I will try this later this evening as it is a more elegant solution :)

@Spitfire972
Copy link
Author

Looking at the code, it isn't possible to use input events to send the update and then using the on() helper to detect it and manage the modifiers. This is because, as part of setting up the x-modelable, the input/change event listener is removed. And if I were to add it back then it would cause the the x-model to capture input events (which is unwanted behaviour).

I could register some custom event listener on the x-model which x-modelable could interact with. However, I don't think this is a good idea as I couldn't find any other examples of a custom event in the code, and all it would do is save repeating the debounce registering code in two places (on() and now x-model)

@ekwoka
Copy link
Contributor

ekwoka commented May 7, 2024

I do actually think switching to an event is the smart way.

Especially for it to support the x-model not being on the same element as the x-modelable. which can work nicely with components

@Spitfire972
Copy link
Author

Spitfire972 commented May 7, 2024

I do actually think switching to an event is the smart way.

Especially for it to support the x-model not being on the same element as the x-modelable. which can work nicely with components

Is there pesident for what I should name the event because it cant be input as that would cause the x-model to act normally. I do think that the ability for it to bubble up is useful though. For now I will name it something like "x-modelable-input" and quickly implement it, but welcome to suggestions

edit:
The "bubbling" up feature won't work as x-modelable specifically checks if x-model is on the element before initialising

@Spitfire972
Copy link
Author

Latest commit uses an event based system.

I am happy to switch this PR to either. Both have their flaws and neither is particularly eligant.

setWithModifiers:

With the new setter you are repeating the debounce/throttle handler code in another place (although I think this could be fixed by abstracting this code into a seperate function so it is clear what it's purpose is. It also adds a new item to the _x_model object on the element which may not be ideal.

Event based:

For the event based system you are forced to register a new event. It is mildly less performent as an event is being dispatched for every update. And the benifits interms of the event being able to bubble is lost by the fact that x-modelable explicitally checks whether there is also an x-model attribute on the element, (and implementing it to allow bubbling would be impossible for setters as you do not know where the child with x-modelable is in the DOM e.g. there could be multiple etc.) (also if this was the behaviour you wanted then it would be better to use $dispatch and x-on or just move the x-model lower in the tree)

Personally, I prefer using the setter (and it's what I will continue to use in my projects for the time being). If this is the prefered option then I would also like to abstract out the throttle/debounce handler code (or maybe the whole handler code at the start of on(), which also supports once, prevent, and stop

@SimoTod
Copy link
Collaborator

SimoTod commented May 7, 2024

For your project, is there a reason why you can't debounce the internal x-model (just to understand the use case)?

@Spitfire972
Copy link
Author

(wrong account sorry - reposting)

For your project, is there a reason why you can't debounce the internal x-model (just to understand the use case)?

I have a min-max slider which are already being modelled with two internal values. To prevent the sliders from moving too close to one another I also have some code binding to the input event. There are 2 different locations which the user can enter values. The slider and a set of number input boxes (which is why x-model internally is alot easier than repeated event bindings and dispatches for each input).

I then want to debounce this input back out to a set of search filters (and also be able to send values back in - hence using model). I originally used events but it was clunky and hard to read as well as having to bind events to the window due to the layout of the input. I therefore also had to add special logic to make sure the events where coming from the right min-max slider. A mess!

Now the code looks something like the following:

<div class="w-96 max-w-full"
    x-data="minMaxSlider(<?php echo $minPrice; ?>, <?php echo $maxPrice; ?>, <?php echo $gap; ?>)"
    x-init="setMinValue(minPrice); setMaxValue(maxPrice)"  x-modelable="[minValue, maxValue]" x-model.debounce="[minPrice, maxPrice]">
    <div class="flex justify-between mb-4">
        <div>
            <label for="min-price-text-input">min</label>
            <input id="min-price-text-input" type="number" min="<?php echo $minPrice; ?>" max="<?php echo $maxPrice; ?>"
                value="<?php echo $minPrice; ?>" step="<?php echo $step; ?>" @input="
                    setMinValue($el.value);
                    $el.value = minValue;
                " x-model="minValue">
        </div>

        <div>
            <label for="max-price-text-input">max</label>
            <input id="max-price-text-input" type="number" min="<?php echo $minPrice; ?>" max="<?php echo $maxPrice; ?>"
                value="<?php echo $maxPrice; ?>" step="<?php echo $step; ?>" @input="
                setMaxValue($el.value);
                $el.value = maxValue;
            " x-model="maxValue">
        </div>

    </div>
    <div class="relative">
        <div class="h-1 w-full bg-grey relative">
            <div class="h-full inset-0 absolute rounded bg-black"
                :style="`left:${((minValue - min) / (max-min)) * 100}%; right:${100 - ((maxValue - min) / (max-min)) * 100 }%;`">
            </div>
        </div>
        <div class="absolute w-full h-1 top-1/2 left-0 -translate-y-1/2 pointer-events-none">
            <label for="min-price-slider-input" class="hidden">minimum price</label>
            <input id="min-price-slider-input" class="absolute w-full inset-0 h-1 pointer-events-none appearance-none"
                style="background:none" type="range" min="<?php echo $minPrice; ?>"
                max="<?php echo $maxPrice; ?>" value="<?php echo $minPrice; ?>" step="<?php echo $step; ?>" @input="
                    setMinValue($el.value);
                    $el.value = minValue;
                " x-model="minValue">
        </div>
        <div class="absolute w-full h-1 top-1/2 right-0 -translate-y-1/2 pointer-events-none">
            <label for="max-price-slider-input" class="hidden">maximum price</label>
            <input id="max-price-slider-input" class="absolute w-full h-1 inset-0 pointer-events-none appearance-none"
                style="background:none" type="range" min="<?php echo $minPrice; ?>"
                max="<?php echo $maxPrice; ?>" value="<?php echo $maxPrice; ?>" step="<?php echo $step; ?>"
                @input="
                    setMaxValue($el.value);
                    $el.value = maxValue;
                " x-model="maxValue">
        </div>
    </div>
</div>

with this JS

class MinMaxSlider {
    constructor(min, max, gap) {
        this.min = min;
        this.max = max;
        this.gap = gap;

        this._maxValue = max;
        this._minValue = min;
    }

    get maxValue() {
        return this._maxValue;
    }

    setMaxValue(value) {
        value = parseFloat(value);
        if (value - this._minValue < this.gap) {
            this._maxValue = this._minValue + this.gap;
        } else if (value > this.max) {
            this._maxValue = this.max;
        } else {
            this._maxValue = value;
        }
    }

    get minValue() {
        return this._minValue;
    }

    setMinValue(value) {
        value = parseFloat(value);
        if (this._maxValue - value < this.gap) {
            this._minValue = this._maxValue - this.gap;
        } else if (value < this.min) {
            this._minValue = this.min;
        } else {
            this._minValue = value;
        }
    }
}

(edited for brevity)

I also think that (even if my use case is insuficient), either a docs change or a code change is required. The docs specifically state that

It's [x-modelable] useful for abstracting away Alpine components into backend templates and exposing state to the outside through x-model as if it were a native input.

and currently this is not how it behaves

@SimoTod
Copy link
Collaborator

SimoTod commented May 7, 2024

The point is that this PR, not being very robust (as you said, both approaches as you said are currently sub-optimal), needs a lof of demand to be merged. Especially because there's a way to do it in userland.
Not exactly the same but what you described can be achieved by putting the debounce modifier on the internal input i.e. x-model.debounce="minValue".
I appreciate they are not debounced between each other but a user won't be that fast anyway.

Not relevant to the whole discussion but instead of adding an @input on each input field (which competes/conflicts with x-model) you should look at using x-effect on a single element to call your functions. x-model is already doing 2 ways binding so stuff like $el.value = maxValue doesn't make much sense.

@Spitfire972
Copy link
Author

Spitfire972 commented May 7, 2024

The point is that this PR, not being very robust (as you said, both approaches as you said are currently sub-optimal), needs a lof of demand to be merged. Especially because there's a way to do it in userland. Not exactly the same but what you described can be achieved by putting the debounce modifier on the internal input i.e. x-model.debounce="minValue". I appreciate they are not debounced between each other but a user won't be that fast anyway.

Not relevant to the whole discussion but instead of adding an @input on each input field (which competes/conflicts with x-model) you should look at using x-effect on a single element to call your functions. x-model is already doing 2 ways binding so stuff like $el.value = maxValue doesn't make much sense.

not relevant to this PR but I found that without $el.value = maxValue it failed to update to the maximised value for that given element. I think this is because my code gets run, then the input event is run on x-model which reset the value prior to when my code ran. I don't see an issue with an input event on the element as they will each handle the event seperately and dont opperate on the same values (there is no setter on the maxValue).

As for using debounce on the inner x-model - this is not a very elegant result for the user, as when they move a slider they get no visual feedback from the number input, e.g. how will they know what value they are currently at, isn't it jaring when the slider stops and suddenly the number changes. I didn't take this solution as I knew the client of the website would have asked me to make it smooth.

To the PR itself. The more I think about it the more I think the original way is correct and I apologise for the confusion. The actual change is very minor and it has been wrapped up in more words than are neccassary because I have been overthinking the whole thing.

I have reverted the change to how I think it should be. The only modifications have been:

  • adding setWithModifiers to x-model
  • using setWithModifiers in x-modelable
  • moving the addition of debounce and throttle modifiers to their own function (optional)

Overall pros

  • x-modelable behaves how it is expected to from the documentation

Overall cons

  • _x_model has an extra member which I guess could be a pro or a con (personally I would see this as a pro but leaving it here)

I apologise for the confusion. I hope you can see the intended original purpose of the PR and the minimal effect this has other than fixing a bug and adding some tests. The code has about 4 lines that have been changed. It is robust (I have been using it this past week), I just didn't have the confidence to stick to my original solution.

@ekwoka
Copy link
Contributor

ekwoka commented May 8, 2024

The "bubbling" up feature won't work as x-modelable specifically checks if x-model is on the element before initialising

Yeah, it would be a more major rewrite of the whole feature...

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

3 participants