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

Remove ["properties"] expression #6291

Open
anandthakker opened this issue Mar 7, 2018 · 4 comments
Open

Remove ["properties"] expression #6291

anandthakker opened this issue Mar 7, 2018 · 4 comments
Labels
api 📝 breaking change ⚠️ Requires a backwards-incompatible change to the API cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.)

Comments

@anandthakker
Copy link
Contributor

The existence of the ["properties"] expression means that the Feature interface we use must provide access to a full map of a feature's properties, rather than just a getPropertyValue(key). This could be a deceptively costly operation. Removing this expression would let us to narrow the feature interface and avoid an unnecessary trap for expression authors. Are there any use cases that can only be served by a ["properties"] expression (instead of just ["get", key] and ["has", key])?

@anandthakker
Copy link
Contributor Author

cc @lucaswoj @GretaCB

@anandthakker anandthakker added breaking change ⚠️ Requires a backwards-incompatible change to the API api 📝 cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) labels Mar 7, 2018
@lucaswoj
Copy link
Contributor

lucaswoj commented Mar 7, 2018

I asked @peterqliu @ryanbaumann @kalimar and @Kilatsat if they'd ever used ["properties"]. None of them had encountered a use case yet.

@1ec5
Copy link
Contributor

1ec5 commented Mar 9, 2018

Since a list of feature properties isn’t declared at the source level (mapbox/tilejson-spec#14), it may be useful to find property names matching a particular pattern. Localization may call for this kind of flexibility at some point in the future, since it’s customary for localized names to be defined on keys named with a finite but large set of ISO language codes. (See #6197 for an idea of how complicated things could get.)

I agree that properties as written could be a performance trap for developers. A couple ideas for alternatives:

  • Somehow make the properties operator lazy so that the map is only fully computed when calling has or get on the result of properties.
  • Replace the properties operator with a property-names operator that returns an array of property names defined on a feature.

@jfirebaugh
Copy link
Contributor

Style authors are amazingly innovative and I'm sure they'll find a use for ["properties"] where nothing else would do. I'm more inclined to discourage misuse by providing a linting tool which could detect things like ["get", "foo", ["properties"]], suggest the replacement, and explain why it's an improvement.

#6252 suggests another kind of static analysis that's more sophisticated than what we may be able to offer at runtime, and is also something that a linter could provide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api 📝 breaking change ⚠️ Requires a backwards-incompatible change to the API cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.)
Projects
None yet
Development

No branches or pull requests

4 participants