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 loadLayerData to intelligently handle multiple requests for the same resource as efficiently as possible #84

Open
Ry-DS opened this issue Jun 18, 2020 · 3 comments
Assignees

Comments

@Ry-DS
Copy link
Collaborator

Ry-DS commented Jun 18, 2020

This is a continuation of the concerns raised by @bencpeters at the following PR disscussion.

In summary, the way BoundaryLayers will load after #81 merges is less than ideal since it is done in MapView. We ideally want a way to load it in BoundaryLayer. If you have a solution, let us know!

Also, after exploring issue #27, I noticed loadLayerData does not have any way to prevent loading things twice or loading while the promise is pending. The current measurements that ensure this doesn't happen are located within the dispatch itself (example).

There is an article on Redux Docs on how to achieve this.

I've already attempted to solve this here, but the implementation isn't done too well - we mutate an object that just lives alongside the thunk. If we can agree this is a worthwhile issue, I can continue on this branch and make a solution that instead works around redux states.

@bencpeters
Copy link
Collaborator

I like the merged "belt & suspenders" solution of having a (essentially no-op) loader in BoundaryLayers in case the data isn't loaded already. This fits the general paradigm of caching the layer data in the first place - e.g. ensure that it's loaded before you use it, but don't care when it's actually loaded.

As far as handling loadLayerData with pending states goes - I don't think we want to cancel the second request (since that could result in issues if the client is awaiting the action and assuming the data will be available when the load promise completes), but we want the second request to grab on to the same pending action and be resolved with the same data as the first, rather than issuing its own request. Perhaps we could store a list or hash of pending requests in Redux and check that before issuing a new request - if a matching pending request exists, the second request just awaits that, and then provides the same data, otherwise it issues its own request.

Unfortunately, there's one other complication: in the truly general case, I don't think id is quite enough to unique identify a layer request. Right now, layer requests are uniquely layerId, date, extent (in theory some layers down the road might even have additional params required for uniqueness, but I think we can probably ignore that for now). We might be able to dodge the extent question for now (although I think that has the potential to cause issues if the user zooms in, then adds a new layer to the map -> IIRC the extents requested with the data are based on the current map view, so if the map is zoomed in, only a subset of the data required for a zoomed-out map will be requested). At the very least we should check layerId, date uniqueness though.

@Ry-DS
Copy link
Collaborator Author

Ry-DS commented Jun 21, 2020

I like the merged "belt & suspenders" solution of having a (essentially no-op) loader

Yes that would work great here! I'm not sure if you recall, but this addition was never merged because of the fear that if BoundaryLayer managed to render before the boundary dispatch from MapView finished loading, it'll issue another load. I plan to add this once the bulk of this issue is solved: handling multiple uniform dispatches to loadLayerData.

I don't think we want to cancel the second request

That's very true. We can look into storing pending promises within the map state slice to check from whenever a new dispatch comes in.

we want the second request to grab on to the same pending action and be resolved with the same data as the first,

This could probably be coded directly into the code for loadLayerData without the need to use condition, middleware or anything fancy (Correct me if I'm wrong).

Unfortunately, there's one other complication: in the truly general case, I don't think id is quite enough to unique identify a layer request.

I'll make sure to check these too. I'll try give this a go after the UI issues we are currently working on are complete.

@Ry-DS Ry-DS changed the title Improve Code Quality of Boundary Layers and loadLayerData Allow loadLayerData to intelligently handle multiple requests for the same resource as efficiently as possible Jul 1, 2020
@wadhwamatic
Copy link
Member

@ericboucher - I have no idea if this issue is still something that needs to be addressed. Assigning to you, but please close it if this isn't relevant anymore

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

No branches or pull requests

4 participants