preact

preact copied to clipboard

This should fix #4161.

The idea is to use a global counter that will increment on event dispatch only, so it’s less likely that the globalCounter will overflow and the order of _attached/_dispatched is going to be guaranteed.

_{tachometer-reporter-action v2 for Benchmarks}

Yes, I tested issue #3927 that was closed by #4126. Works fine.

coverage: 99.47% (-0.1%) from 99.601% when pulling 1749ffdd0ce2282f480d4a909a0fe1de60fc4cb0 on nbalatsk-oracle:main into bc7c5515599634a427334eb03236ddf19e13e618 on preactjs:main.

Hi guys, I've refreshed my branch with the latest code. Let me know if you have any comments.

@JoviDeCroock I have seen that you have gone for the 32-bit integer solution, as opposed to just counting from Number.MIN_SAFE_INTEGER upwards.

With 32-bit integers, there is a slight chance that it rolls over if many events are fired and if you have a system that runs for a long time (kiosk systems?). If I understand the code correctly, every event attached before MaxInt32 wouldn't be fired after rolling over.

Counting upwards from Number.MIN_SAFE_INTEGER however, you would practically never reach Number.MAX_SAFE_INTEGER. Even with 10,000 events a second, this would take 57,123 years.

@sschultze this isn't my PR just my comment 😅

@JoviDeCroock Sorry 😅

@nbalatsk-oracle @JoviDeCroock What about using BigInt for the counter if the browser supports it? BigInts have an arbitrary size and will never overflow, and also no strange things happen if they get very large, giving some peace of mind.

The performance cost should be negligible. On my computer, 10 million increments of a BigInt take 151ms.

Initialization for the counter could go like this:

let counter = window.BigInt ? BigInt(0) : 0;

The ++ operand and the comparison would work fine on both number and bigint, so no need for another case distinction.

@ritschwumm @JoviDeCroock do we want to use BigInt for the counter as suggested by @sschultze? Do you any concerns for performance?

@nbalatsk-oracle can't the rollover be made negligible by just comparing for equality with the previous value? then the only problem left is when 2^32 events happen "at the same instant"...

Probably another issue, but isn't it problematic that, even in the original code, the _attached property is added to the event handler function? The same event handler might be added to multiple nodes, potentially leading to internal mess-up. Even I re-use event handlers in my code (e.g. for list items differentiated by a data-key attribute), and my UI is not that special.

I also noticed that in the minified version, _attached is shortened to u. Couldn't it happen that there's a conflict between the minified name and the "official" properties of the function?

@nbalatsk-oracle can't the rollover be made negligible by just comparing for equality with the previous value? then the only problem left is when 2^32 events happen "at the same instant"...

@ritschwumm I don't think that's how the current proposed code changes are working. The idea is that the counter is incremented when an event listener is newly attached for the first time during a render cycle. All event listeners that are newly attached during that render cycle will share the same counter value; all event listeners attached during previous renders will have lower counter values, all event listeners attached in future renders will have higher counter values. As a result, event listeners attached during the 0th render cycle appear indistinguishable from event listeners attached during the 2^32nd render cycle, hence the "rollover" issue.

Probably another issue, but isn't it problematic that, even in the original code, the _attached property is added to the event handler function? The same event handler might be added to multiple nodes, potentially leading to internal mess-up. Even I re-use event handlers in my code (e.g. for list items differentiated by a data-key attribute), and my UI is not that special.

@sschultze Is it really a problem? It seems that it does not really matter that the handler is shared as long as the _attached/_dispatched order is correct. Even if _attached is updated by some DOM mutation (new elements are added that share the same handler), the event will not propagate to those updated elements. What is the use-case where we want it?  And if you trigger an event on one of those older elements the event should work because the order of the _attached/_dispatched pair is still correct. Right?

@sschultze Is it really a problem? It seems that it does not really matter that the handler is shared as long as the _attached/_dispatched order is correct. Even if _attached is updated by some DOM mutation (new elements are added that share the same handler), the event will not propagate to those updated elements. What is the use-case where we want it?  And if you trigger an event on one of those older elements the event should work because the order of the _attached/_dispatched pair is still correct. Right?

@nbalatsk-oracle You are right. Even though adding the same event handler in multiple places was probably not thought of when writing the original _attached/_dispatched workaround (or perhaps it was thought of, i don't know), in practice this shouldn't be a problem as the order will still be correct.

I mean, I can't say for sure - I'm not deep enough into the internal workings of Preact to guarantee that there isn't some weird edge case where still would still create a problem.

I did however write the following test code which re-uses the same event handler handleButtonClick and aggressively creates new DOM nodes, just for a quick validation, and it works fine. No click event is ignored.

import { JSX, useCallback, useState } from 'preact/compat';

interface SectionProps {
    readonly onButtonClick: JSX.MouseEventHandler<HTMLButtonElement>;
}

function Section(props: SectionProps) {
    return (
        <div>
            <button onClick={props.onButtonClick}>Add Section</button>
        </div>
    );
}

let domNodeCreationCounter = 0;

export function PreactTest(props: {}) {
    const [count, setCount] = useState(1);

    const handleButtonClick = useCallback(() => {
        setCount(oldCount => oldCount + 1);
    }, []);

    // console.log((handleButtonClick as any).u); - shows ._attached in the minified version that I use

    return (
        <div key={domNodeCreationCounter++}> {/* Should create new DOM nodes every time */}
            {Array(count).fill(0).map((_, index) => (
                <Section key={index} onButtonClick={handleButtonClick} />
            ))}
        </div>
    );
}

Perhaps it would make sense to add a "warning comment" for anyone who updates the code later? Saying that the property is added to the function itself, even though it might be added to multiple nodes, but as you say this still doesn't create a problem as the _attached/_dispatched order is still okay?

@nbalatsk-oracle A thought experiment: If it would indeed be 100% safe to share event handlers between nodes, and still always get the correct behavior here, then it would - as a consequence - also be safe to share the same one event handler for everything in the application. Meaning there's really only one event handler function for everything in the application in this theoretical scenario.

In this scenario, the _attached property would always be set in the same function object.

If this would still lead to the correct behavior, than _attached could be a (module-)global variable, and not be attached to the event handler functions in the first place.

What I want to say by this is:

• either event handlers can really be shared between nodes and still get 100% correct behavior, in which case _attached could also be made a (module-)global variable
• or neither the original nor the corrected workaround provide a 100% solution.

In any case, especially if we get the overflow problem away (e.g. with the bigint proposal), the new code definitely provides better results that the old code relying on Date. (The Date-based workaround not only makes problems in the test scenario, but also if the users changes their system date/time while the app is running.)

Updated the code to use BigInt for the counters to avoid overflow and added a few comments to _attached/_detached usage.

@JoviDeCroock @developit @andrewiggins @marvinhagemeister Just curious if you might be able to provide any indication on the likelihood of moving forward with the approach in this PR or if there are other approaches to this problem that you might like to see investigated? Our product is currently blocked from moving forward from preact 10.17.1 due to https://github.com/preactjs/preact/issues/4161 so we're very interested in finding some acceptable resolution. Happy to explore other options if there's a preference, thanks!