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

Calculation of widget using maps API #658

Conversation

stefano-xy
Copy link
Contributor

@stefano-xy stefano-xy commented Apr 25, 2023

Description

Shortcut: https://app.shortcut.com/cartoteam/story/304698

This introduces the possibility for widget to be calculated remotely, so using the maps API. It is the case today for global mode only. This PR adds supports for non-global widgets to offload calculation to the backed. This is also limited to non-tileset data source and data source using V3-API only.

When enabled, widgets are more reactive and don't need to wait for the complete dataset to be downloaded. In demo apps, widgets gets rendered before data is even shown on the map.

The point of control of the feature is the function isRemoteCalculationSupported. Note that each widget must have this possibility turned on as well, via the initialisation parameter attemptRemoteCalculation, currently hardcoded to true for widgets that supports the feature.

No change in public API expected. Carto 4 React users won't be able to control this feature.

@shortcut-integration
Copy link

@coveralls
Copy link
Collaborator

coveralls commented Apr 25, 2023

Pull Request Test Coverage Report for Build 5016122094

  • 38 of 50 (76.0%) changed or added relevant lines in 13 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 72.11%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/react-core/src/filters/tileFeaturesSpatialIndex.js 0 1 0.0%
packages/react-redux/src/slices/cartoSlice.js 3 4 75.0%
packages/react-widgets/src/hooks/useWidgetFetch.js 6 7 85.71%
packages/react-widgets/src/models/HistogramModel.js 5 6 83.33%
packages/react-api/src/api/model.js 3 5 60.0%
packages/react-api/src/hooks/useGeojsonFeatures.js 2 5 40.0%
packages/react-api/src/hooks/useTileFeatures.js 2 5 40.0%
Totals Coverage Status
Change from base Build 5012017966: 0.2%
Covered Lines: 2034
Relevant Lines: 2600

💛 - Coveralls

@stefano-xy stefano-xy self-assigned this Apr 27, 2023
@stefano-xy stefano-xy force-pushed the feature/sc-304698/backend-based-widget-calculation-using-spatialfilter branch from 90a42a9 to b6d0241 Compare May 2, 2023 12:50
@stefano-xy stefano-xy force-pushed the feature/sc-304698/backend-based-widget-calculation-using-spatialfilter branch from c48b227 to 32f47cc Compare May 4, 2023 08:32
Copy link
Contributor

@zbigg zbigg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

const isGet = url.length + JSON.stringify(queryParams).length <= URL_LENGTH;
if (isGet) {
url += '?' + new URLSearchParams(queryParams).toString();
} else {
// undo the JSON.stringify, @todo find a better pattern
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't get much better because you have to measure size of GET url few lines above and that must have proper url as input.

(btw, it should create proper URL first, measure and test and not rely of more less accourate assumptions that JSON.stringify(queryParams). is same length as new URLSearchParams(queryParams).toString() but that's other story)

This is done correctly in getStats albeit it's not much more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to introduce an utility function for this, in a later PR.

@stefano-xy
Copy link
Contributor Author

Im my opinion, we're ready to merge this PR.

@VictorVelarde VictorVelarde force-pushed the feature/sc-304698/backend-based-widget-calculation-using-spatialfilter branch from 0c015e0 to 0c18699 Compare May 18, 2023 16:35
Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 really nice job here!

@VictorVelarde VictorVelarde merged commit 02b9be5 into master May 18, 2023
@VictorVelarde VictorVelarde deleted the feature/sc-304698/backend-based-widget-calculation-using-spatialfilter branch May 18, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants