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

Include GeoJSON id in tree item, return whole item #6

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bhousel
Copy link

@bhousel bhousel commented Apr 6, 2018

I have a bunch of GeoJSON Featues without properties, but they each have an id (see RFC7946)

Can we change this library to:

  • include the id in the cached tree items (if not undefined)
  • return the whole tree item in the results, not just the props

This is an API breaking change, but it would be great to support id lookup.

@bhousel bhousel requested a review from mourner April 6, 2018 02:26
@andrewharvey
Copy link
Contributor

Thanks @bhousel I so happened to need to do this today. I tested your branch out, it looks better, but in line with how geokdbush works by returning the original item would it be better to simply return the whole Feature object as is from the provided GeoJSON.

It makes it easier to pass the polygon around if the return value is a GeoJSON Feature.

@mourner
Copy link
Member

mourner commented Apr 20, 2018

Yeah, thinking back on my past API decision, I think I should have made it return the whole features.

@bhousel
Copy link
Author

bhousel commented Apr 20, 2018

Yeah, thinking back on my past API decision, I think I should have made it return the whole features.

If you are ok with it, I'm happy to merge this and release a v1..
(I'd also move this project into the mapbox scope and update whatever dependencies it has)

@andrewharvey
Copy link
Contributor

Or if we are going to make a breaking change, we make it return the original feature, rather than just the id and properties?

@bhousel
Copy link
Author

bhousel commented Apr 20, 2018

Or if we are going to make a breaking change, we make it return the original feature, rather than just the id and properties?

Oh I see what you mean - push the whole original feature into treeItem. I think that might be useful, but it would also do a strange thing with MultiPolygons. (This library currently breaks them up and caches each polygon separately).

@mourner
Copy link
Member

mourner commented Apr 20, 2018

Oh, that might have been the original reason I didn't push the original items to the tree. Let me think this through...

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

Successfully merging this pull request may close these issues.

3 participants