Fix event bubbling can break in test conditions (#4161) #4232
Conversation
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Results02_replace1k
duration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
03_update10th1k_x16
duration
usedJSHeapSize
07_create10k
duration
usedJSHeapSize
filter_list
duration
usedJSHeapSize
hydrate1k
duration
usedJSHeapSize
many_updates
duration
usedJSHeapSize
text_update
duration
usedJSHeapSize
todo
duration
usedJSHeapSize
|
src/diff/props.js
Outdated
| @@ -13,6 +13,11 @@ function setStyle(style, key, value) { | |||
| } | |||
| } | |||
|
|
|||
| let globalCounter = Number.MIN_SAFE_INTEGER; | |||
| const getCounter = increment => { | |||
| return increment ? ++globalCounter : globalCounter; | |||
There was a problem hiding this comment.
I don't think it's safe to assume that the user has a certain amount of events, if we however memoize this like the example I gave in the issue that would become a lot safer as then the assumption becomes amount of re-renders instead of amount of events.
There was a problem hiding this comment.
maybe force globalCounter to be a 32 bit int using globalCounter | 0? as far as i know javascript VMs, it would be a tad bit faster.
additionally it would at least lead to predictable overflows whereas a regular double would start doing interesting things at around 2^52.
There was a problem hiding this comment.
Take a look at the update. Used the counter technique mentioned in the issue, forced counters to be unsigned int with >>> 0.
There was a problem hiding this comment.
do i miss something that makes >>> 0 better than | 0? it's longer and halves the available range...
There was a problem hiding this comment.
I agree that it is uglier, but if I understand it correctly the shift operator converts a number to an unsigned 32 bit int with the range of [0 to 4294967295]. The bitwise OR converts a number to a signed 32 bit int with the range of [-2147483648 to 2147483647].
When we start the counter with 0 the unsigned int doubles the range. Right?
You can check it with code like this:
const x = 0b1111111111111111111111111111111 >>> 0; // 2^31 -1 as unsigned int32
const y = 0b1111111111111111111111111111111 | 0; // 2^31 -1 as signed int32
console.log(x, y);
console.log(x + 1 >>> 0, y + 1 | 0);
I've read about the signed/unsigned manipulation in this article. I wonder if we want to use those manipulations at all?
There was a problem hiding this comment.
i think i misunderstood what >>> does and unsigned should have exactly the same range size as signed (both are 32 bits in the end...). i definitely would use integers instead of floating point, it just feels a lot better :)
There was a problem hiding this comment.
Have you been able to test this with the issues that were closed by #4126
JoviDeCroock
requested review from
developit,
andrewiggins and
marvinhagemeister
|
Hi guys, I've refreshed my branch with the latest code. Let me know if you have any comments. |
There was a problem hiding this comment.
The thing bothering me most about this approach is when it rolls over (rare case) we might have event-handlers registered with MAX_INT_32 and 0 which all of a sudden would incorrectly bail out of a bubbled click 😅 thinking about alternatives like registering event-handlers during the commit phase but not sure as of just yet because commit is still triggered synchronously so the microTick delay between bubbling could make the bug re-appear.
The initial solution we took was to delay state-setting by a tick rather than a microtick which would successfully make it trigger after bubbling completes https://www.jovidecroock.com/blog/browser-timings however that had its own set of issues
We can see this working in the original issue: https://codesandbox.io/p/sandbox/wandering-cdn-4s53z4?file=%2Fsrc%2Fpreact.js%3A726%2C1 Also has still fixed the original issue of the event timings https://codesandbox.io/p/sandbox/amazing-rhodes-xpj62v?file=%2Fsrc%2Findex.jsx%3A10%2C6
|
@JoviDeCroock I have seen that you have gone for the 32-bit integer solution, as opposed to just counting from 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 |
|
@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:
The |
|
@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 I also noticed that in the minified version, |
@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. |
@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 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 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 If this would still lead to the correct behavior, than What I want to say by this is:
In any case, especially if we get the overflow problem away (e.g. with the |
nbalatsk-oracle
force-pushed
the
main
branch
from
5b686ca to
d6077a6
Compare
|
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 #4161 so we're very interested in finding some acceptable resolution. Happy to explore other options if there's a preference, thanks! |
| // It is supported by all major browsers except IE and Opera mini | ||
| // where the plain number is used as fallback. | ||
| // eslint-disable-next-line no-undef | ||
| const zero = typeof window != 'undefined' && window.BigInt ? BigInt(0) : 0; |
There was a problem hiding this comment.
| const zero = typeof window != 'undefined' && window.BigInt ? BigInt(0) : 0; | |
| const zero = Number.MIN_SAFE_INTEGER; |
| @@ -120,7 +120,7 @@ declare global { | |||
| } | |||
|
|
|||
| export interface PreactEvent extends Event { | |||
| _dispatched?: number; | |||
| _dispatched?: number | bigint; | |||
There was a problem hiding this comment.
| _dispatched?: number | bigint; | |
| _dispatched?: number; |
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.