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

refactor(decodeValues): short circut out of decoding values if there are no CVD fields #394

Merged
merged 1 commit into from
Nov 19, 2018

Conversation

tomwayson
Copy link
Member

This seemed like potentially hot code, esp as we look at how we are implementing it in cedar/the Hub, so I made the following changes:

  • just return the original response if there are no CVD fields to decode
  • don't clone the response before looping through all features/attributes

AFFECTS PACKAGES:
@esri/arcgis-rest-feature-service

…are no CVD fields

only clone & re-loop query response if there are CVD fields to decode
otherwise just return the
original query response

AFFECTS PACKAGES:
@esri/arcgis-rest-feature-service
@tomwayson tomwayson requested a review from jgravois November 19, 2018 06:45
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 1bac187 on chore/refactor-decode-values into 17899dc on master.

@tomwayson
Copy link
Member Author

@jgravois

To explain why I made these changes. I started thinking about how we're going to use this in the Hub. As we discussed the other day, whenever we create charts/tables in the Hub we have direct access to the layer's metadata (fields), and I figured we'd just update the code there to always call decodeValues() and pass those in. Then I thought that having CVD fields is more the exception than the norm, so there's no reason to clone the response and then loop through the cloned features if there are no CVD fields (the norm).

@tomwayson
Copy link
Member Author

Thinking specifically about the chart card in the Hub, we may want to take this a step further and export extractCodedValueDomains() and allow passing in domains as an alternative to fields to decodeValues().

The reason is that for the chart card, we serialize the card settings needed to create a definition. So now we're going to have to add either fields or domains to those settings, and they will get stored in the page item JSON. We definitely don't want to serialize and store all fields. So at the very least, the Hub would need to filter and serialize only CVD fields. I say we cut out the middleman and let the Hub call extractCodedValueDomains() and then it can just pass and serialize the domains (if any).

Thoughts?

@tomwayson
Copy link
Member Author

as per our discussion, just now let's stick w/ fields at the rest-js level. For charts, cedar knows what field it cares about (the category field) and it can inspect whether or not that has a domain and pass that along.

Copy link
Contributor

@jgravois jgravois left a comment

Choose a reason for hiding this comment

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

  • definitely a useful optimization
  • something i was planning on tackling in the future
  • makes the internal business logic more readable
  • adds another test that targets a very common use case.

nice work!

@jgravois jgravois merged commit 891bfb7 into master Nov 19, 2018
@jgravois jgravois deleted the chore/refactor-decode-values branch November 19, 2018 18:54
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