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

Replace loadGeometry() with a geometry getter #34

Closed
wants to merge 2 commits into from

Conversation

jfirebaugh
Copy link
Contributor

Since we're talking about a major version bump for #33, I'd like to propose this API change as well. Replacing loadGeometry() with a geometry property will make it easier, for testing purposes, to substitute a plain old JS object where a VectorTileFeature would normally be used.

In addition, we have the option of caching the generated geometry internally if we later think that makes sense, without changing the API or ending up with a misleading method name.

@mourner
Copy link
Member

mourner commented Oct 19, 2015

I think we should add caching if we want to make this change, since otherwise it would be easy to misuse — people don't expect a simple property access to be very computationally intensive (in this case, reparsing the whole geometry from scratch), and loadGeometry() indicates better that something's going on under the hood.

I'm divided on this change generally. And it's still as easy to substitute on the testing side:

feature.loadGeometry = function () { return mockGeometry; };

@mourner
Copy link
Member

mourner commented Dec 21, 2015

@jfirebaugh with VT spec 2.0 released, let's move on this PR. Two options to go forward:

  1. Leave feature.geometry as a getter but cache it after parsing so that it doesn't get reparsed on every access.
  2. Revert to public loadGeometry() method.

I'm fine with either but prefer the latter option since it's more explicit.

@mourner
Copy link
Member

mourner commented Mar 16, 2016

Stale.

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.

None yet

2 participants