expose instantiated scales descriptors in the render API #1810
Conversation
…ptor’s range and domain, better align it with what the D3 scales return
src/scales.js
Outdated
| const scaleFunctions = {scales}; | ||
| for (const [key, desc] of Object.entries(descriptors)) { | ||
| const {scale, type, interval, label} = desc; | ||
| scales[key] = exposeScale(desc); |
There was a problem hiding this comment.
This involves a bunch of copying and these exposed scales are rarely used. I think we should investigate whether we can make this lazy using a getter (but probably a caching getter so that if you access the same scale multiple times it returns the same instance).
There was a problem hiding this comment.
Fil’s comment: We’ll need these exposed scales in the near future anyway when we adopt them internally for axes etc., and therefore it’s premature optimization (or even slower) to make these lazy.
test/plots/render.ts
Outdated
| import * as d3 from "d3"; | ||
| import * as htl from "htl"; | ||
|
|
||
| export async function renderInstantiatedScales() { |
There was a problem hiding this comment.
The file name “render.ts” isn’t very descriptive.
And I think this would be better as a unit test instead of a snapshot test since most of it is about checking the exposed scales, not the visual appearance of the chart.
|
I like the progressive roll-out plan, starting here and then following up with tightening down the scale function representation to just the apply function. 👍 |
There was a problem hiding this comment.
This also changes plot.scale(…) to return the existing instantiated scale instance, rather than creating a new instance each time you call it. I think that’s okay, but we should note that in the release notes and the documentation since it’s a slight change in semantics. Possibly we should still make a defensive copy to protect against external mutation, but… I dunno, maybe it’s better to minimize the number of copies instead.
* expose instantiated scales descriptors in the render API * more uniform handling of identity scales * don’t expose domain and range for identity (yet) --------- Co-authored-by: Mike Bostock <mbostock@gmail.com>
…q#1810) * expose instantiated scales descriptors in the render API * more uniform handling of identity scales * don’t expose domain and range for identity (yet) --------- Co-authored-by: Mike Bostock <mbostock@gmail.com>
Address the most important part of #1263. In this simplest approach, we add the instantiated scales descriptors to the otherwise unmodified scales object.
the render method is thus called as
where xScale, yScale, colorScale (etc) are D3 scales, and x, y, color are instantiated scale objects.
xScale, yScale… are documented as functions that map from the data domain to the visual domain (the apply function), but still leak the D3 abstraction. The long-term plan is to replace them with the apply function only; this means chasing all the internal occurrences — for example, of xScale.bandwidth() or xScale.ticks() — and replacing them with a proper look-up in the scale descriptor (scales.x.bandwidth). (It will be a different PR.)