-
Notifications
You must be signed in to change notification settings - Fork 25
Debounce/throttling support #10
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
Comments
Hi, @wrench7! Thanks for your kind words. I've thought about this feature, and have something similar listed in the Roadmap. Implementation SketchThe steps would be something like this:
There are some tricky bits that might make this hard to get right:
I'll play around with this and see how it shapes up. Here's a simple test case to check any implementations with: |
Wow, what a fantastic response! :-) Your implementation sketch pretty much nails it I think. The test case looks good as well. Bit of an aside, but I could have sworn I've seen Chrome cancelling image requests when components are removed, but can't seem to see that in the dev tools now, nor find any results when googling for things along the lines of "react cancel image load". That may not be relevant I guess if all you're thinking about doing is cancelling the initial Delay Promise, and once things have progressed to the Image Promise it's going to load regardless of where the viewport is. If you were going to go down the promise cancelling path, this appears to be a sound approach: https://stackoverflow.com/a/37492399 Let me know if I can help out in anyway as well! :-) EDIT: I just made the changes to the underlying code and used a fairly simplistic setTimeout/clearTimeout for the delay aspect. Code changes were minimal going down this path. |
Hi @wrench7, I managed to get it working 🎉 The code at https://codesandbox.io/s/3215xr7615 uses Release timingI have a few more features pending for that release ( External APIThe external interface is the same, with an optional InternalsThanks for the cancellation reference, it was helpful to get the internal API where I wanted. I am not really a fan of promises, but one of the interfaces returns one already (loading the image), so it made sense to me to use one for the delay, simple as it might be. (Their property of always resolving with the same result also seems useful to React, one of their next features depends). I'm curious what the setTimeout/clearTimeout solution looks like. React usually gives me dev warnings about "task scheduled in render" with those 🤔 There was a refactor of the codebase, particularly to isolate areas where we kick off side-effects (buffering, image requests) and make the various states and transitions explicit. It's where it was headed with the internal state, and adding an extra "Buffering" state tipped the scales. The code grew a bit, but I like it much more now; it makes good use of types imo. The source is on the feature-delayed-loading branch, if you're interested: |
Hi Fotis :-) Awesome work with that update. I took the codesandbox sample for a spin and it works very nicely. I'll hold off using the RC until you're happy with the changes. re: the setTimeout solution I hacked together - you're right I now get the occasional "Can only update a mounted or mounting component" error in the console, but this doesn't appear to have an adverse affect on the rendering. Obviously your solution won't have that issue so I look forward to having a play with it. Thanks again! |
Hi! Quick ping to let you know that I've published 1.1.0 on npm. |
Fantastic work! I'm on holidays myself at the moment, but will pull the
update next week and report back if I see any issues with
the debounceDurationMs feature.
Thanks again :-)
EDIT: just installed the latest v1.1 and it works like a charm :-)
|
Firstly, fantastic job with this library! Love that it's built directly in Typescript :-)
Wondering how hard it would be to add a debounce/throttle option so that only images that stay within the viewport for a certain amount of time actually trigger the loading of their content?
The text was updated successfully, but these errors were encountered: