search

farminf / pannellum-react

React Component for Pannellum (open source panorama viewer for the web)
MIT License
112 stars 81 forks source link

Adding in functionality to abort xhr requests #88

Closed btChrisHerzog closed 2 years ago

btChrisHerzog commented 2 years ago

We wanted to add a way to abort an xhr request that had not yet returned with a response before a user of the library was finished with the component.

image

We've elevated the scoping of the xhr variable to be called in the destroy function and called destroy when the component unmounts.

farminf commented 2 years ago

I don't think this is something that this component should be responsible of... this component is about 360 images and videos and not around race conditions on fetch or any HTTP related stuff

SamKirkland commented 2 years ago

Hey @farminf are you open to discussing this a bit?

In our case we wanted to do this as a optimization for our users on slower speed networks. For example a user triggers an action that renders the pannellum-react component but decides to stop waiting because they are on a slow network. Unmounting the component today does not cancel the network request, which means it's silently eating the users bandwidth in the background. This gets really scary when you are on a single page app as the request will never be cancelled, even on page navigations. To further amplify the issue our usage of this component is within a image Lightbox, in the worst case a user could click "next" a number of time, triggering many of these long running requests.

I can see your point about not wanting this library to deal with HTTP related issues. However I view this as a memory management issue rather than a race condition. Components should cleanup after themselves by unbinding event listeners they created, cancelling any promises/API calls, and removing any setTimeout/setInternals. Components should strive to cancel any network requests that have not completed. Generally this is handled with a cancellation token but in this case it can be accomplished by killing the connection. The applications memory should be the same before a component is rendered vs. after it's unmounted (after forcing GC to run and what-not). Today that is not happening 😢

We would be happy to implement this on our side however that's not possible today due to the image prop only accepting a string and all HTTP logic is implemented within the library. If that prop accepted a data stream this could be done by the implementer side instead.

Based on this additional context do you think we can get this reviewed/merged?