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.

Sign up for GitHub

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

Jump to bottom

Wkt support staging #1387

Merged
merged 9 commits into from
Apr 30, 2024
Merged

Wkt support staging #1387

merged 9 commits into from
Apr 30, 2024

Conversation

Bonkles
Copy link
Contributor

@Bonkles Bonkles commented Apr 25, 2024

This PR adds a new wktPoly param that very quickly gets basic POLYGON and MULTIPOLYGON support into Rapid.

@Bonkles Bonkles linked an issue Apr 25, 2024 that may be closed by this pull request
Support arbitrary WKT Polygons in a new URL Param #1386
Closed
Bonkles
Bonkles commented Apr 25, 2024
View reviewed changes
modules/pixi/PixiLayerCustomData.js
@@ -8,6 +8,7 @@ import { PixiFeatureLine } from './PixiFeatureLine.js';
import { PixiFeaturePoint } from './PixiFeaturePoint.js';
import { PixiFeaturePolygon } from './PixiFeaturePolygon.js';
import { utilFetchResponse } from '../util/index.js';
import { parse as wktParse } from 'wkt';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New module import that correctly interprets WKT strings of all kinds.

Bonkles
Bonkles commented Apr 25, 2024
View reviewed changes
Copy link
Contributor Author

@Bonkles Bonkles left a comment

Choose a reason for hiding this comment

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

Left a number of comments to fix things now that I'm in 'review' mode.

modules/pixi/PixiLayerCustomData.js Outdated Show resolved Hide resolved
@bhousel bhousel added this to the v2.3 milestone Apr 25, 2024
bhousel
bhousel reviewed Apr 25, 2024
View reviewed changes
Copy link
Contributor

@bhousel bhousel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I took a quick pass over this, and my initial impression is that it seems like you might be doing too much work?

My mental model of how this might work is like:

  • in _hashchange, take the data param and try to parse it as WKT.
  • If that succeeds, you'll get some blob of GeoJSON
  • call this.setFile(geojson, '.geojson')
  • Done? everything else in this file is already set up to deal with GeoJSON.

@Bonkles
Copy link
Contributor Author

Bonkles commented Apr 26, 2024

I took a quick pass over this, and my initial impression is that it seems like you might be doing too much work?

... everything else in this file is already set up to deal with GeoJSON.

Excellent idea. Refactoring to do exactly this!

in _hashchange, take the data param and try to parse it as WKT.

One downside: printing appropriate error messages with the overloaded data param. I can still wire up the separate wktPoly param and make it use the same code path of this._setFile(geojson, '.geojson'), or I can force err'thang to use 'data'. What do you think?

@bhousel
Copy link
Contributor

bhousel commented Apr 26, 2024

One downside: printing appropriate error messages with the overloaded data param. I can still wire up the separate wktPoly param and make it use the same code path of this._setFile(geojson, '.geojson'), or I can force err'thang to use 'data'. What do you think?

I'd force everything to just use data= (for now)
I can imagine a situation where people want to see a bunch of custom data sources on the screen, e.g. an area of interest and PMTiles, but we aren't set up to support multiple custom data or imagery sources yet (but we should add that later!)

Good callout on the downside of not being able to print appropriate error messages - I think that's probably another thing we don't do at all right now? e.g. if you pass it a bad geojson or PMTiles, I think it just silently fails today? We should probably eventually surface this somehow.

@bhousel bhousel marked this pull request as ready for review April 30, 2024 17:55
@bhousel
Copy link
Contributor

bhousel commented Apr 30, 2024

Looks good! I do think we can simplify further and remove the arbitrary limitation to only include Polygon/Multipolygon types, but let's merge it so people can try it out.

@bhousel bhousel merged commit fa6a54d into main Apr 30, 2024
5 checks passed
@bhousel bhousel deleted the wkt_support_staging branch April 30, 2024 17:58
bhousel added a commit that referenced this pull request Apr 30, 2024
@bhousel
cleanup WKT code
cd29674 
(re: #1387)

All minor stuff:
- move private functions to end of file
- move variable cleanup stuff into a `_clear()` function
- cleanup lint warnings, whitespace
- `parseAsWkt` needs to return `null` if it wants to do nothing
  (before it returned `{}` which evaluates as truthy)
- call .toUpperCase() on the wkt string, so it writes back to the url hash that way
- don't report a `dataUsed` for wkt sources (similar to task boundaries)
- make sure to check `typeof newData === 'string'` in `_hashchange`
  (sending non-string data to the wktparser fails)
- lots of testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bhousel bhousel bhousel left review comments

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
v2.3
Development

Successfully merging this pull request may close these issues.

Support arbitrary WKT Polygons in a new URL Param
3 participants
@Bonkles @bhousel @facebook-github-bot