Strip undefined values from props to avoid unintentionally shadowing defaults

[ericsoco]

Ran into a problem when creating a new layer in which passing props that are undefined shadow defaults, requiring application-layer or CompositeLayers to run checks to avoid passing a prop at all if it's undefined.

E.g.:

class FooLayer extends Layer {
  // ...
  renderLayers() {
    const valueNeededForRender = this.props.someAccessor();
    // ...
  }
}
FooLayer.defaultProps = {
  someAccessor: () => true
};

// application code:
// ...
render() {
  // `someAccessor` not passed in or defined due to some valid condition/use case...
  const {someAccessor} = this.props;
  new FooLayer({
    someAccessor
  });
}

In this case, the expectation is that omitting someAccessor will allow FooLayer to fall back to its default definition for someAccessor. Instead, the default is shadowed by the undefined value passed as a prop, causing a runtime error.

This PR fixes that case, but I can easily imagine it will surface other bugs in which that shadowing is unintentionally expected / accounted for and unshadowing will cause other errors. So...not sure how best to proceed, but figured I'd put this out to see what you think.

[ibgreen]

This is addressing a grey zone of JavaScript semantics. If we make this change we should document it, as it not obvious to me that the API would work this way, and I would want to know that I could depend on it before starting to use it. It does mean that apps need to use null instead of undefined to mark empty values which is a good practice but not everyone does it.

Those doc changes should be part of the PR.

In addition, layer creation speed is important, as apps may need to create a lot of Layer instances 60 times per second.

We actually have benchmarks that measure that (npm run bench). So at least measure the impact of this and update the PR with the results.

[ericsoco]

New layers are instantiated and old ones disposed every frame, so this is on the critical path for performance. In that light, this PR could be refactored to avoid creating an array (Object.keys()) and an object (.reduce()). It's also critical to benchmark the performance impact on layer creation; for this, we can use npm run bench.

Note that this PR came from my experience with React. This SO post highlights React's handling of undefined props vs. defaultProps and this thread contains a conversation between React devs about the intentionality of that handling.

I suggest we sit on this for a bit to see if the risk is worth the reward -- it's a relatively small ergonomic improvement and may be mostly inconsequential. But might also be useful.

Side note: this would also allow application developers to intentionally fall back to layer defaults via this pattern:

new FooLayer({
  someProp: expectedValue || undefined
});

(Some of this echoes @ibgreen's comments above, which landed while I was writing this 😉 )

[ericsoco]

Performance difference is unclear from these tests, but here they are for posterity:

Operation	no removal	for-in removal	keys + filter + reduce removal
color#parseColor (string)	(±2.41%) 933,494	(±2.88%) 938,705	(±2.70%) 964,603
color#parseColor (3 element array)	(±5.03%) 29,357,109	(±1.15%) 34,774,314	(±2.00%) 32,609,815
ScatterplotLayer#construct	(±1.76%) 238,476	(±0.99%) 290,293	(±1.77%) 193,576
ChoroplethLayer#construct	(±1.61%) 353,570	(±1.59%) 393,048	(±1.42%) 255,971
PolygonLayer#construct	(±2.11%) 170,612	(±2.28%) 137,970	(±1.70%) 153,315
ScatterplotLayer#initialize	(±3.02%) 147	(±2.26%) 154	(±2.58%) 154
PathLayer#initialize	(±4.52%) 20.65	(±3.34%) 21.35	(±2.52%) 21.73
ChoroplethLayer#initialize	(±5.19%) 4.78	(±5.61%) 5.46	(±3.96%) 5.59
PolygonLayer#initialize (flat)	(±1.54%) 13,066	(±1.45%) 11,979	(±0.98%) 13,427
PolygonLayer#initialize (extruded)	(±5.95%) 10,382	(±1.97%) 11,893	(±3.64%) 11,479
PolygonLayer#initialize (wireframe)	(±7.62%) 11,133	(±3.49%) 10,615	(±2.80%) 11,375
ExtrudedChoroplethLayer64#initialize	(±3.19%) 21,453	(±3.38%) 20,115	(±4.49%) 16,378

[ibgreen]

Thanks for collecting these.

It is mainly three Laye#construct cases that are of interest here.

Interestingly, your (optimized) change seems to increase speed in 2 of 3 cases, which seems promising. (Note that there is a bit of run-to-run variability in these benchmarks due to unpredictable machine load.)

It would be good to add test and benchmark cases for more layers (ideally driven by a table), and for various numbers of props (a short list of props will of course be faster to resolve than a longer list).

[ericsoco]

Closing for now as benefits are debated and perf impact is indeterminate.