[Drawer] - Exported types are possibly a little odd

palantir / plottable

:bar_chart: A library of modular chart components built on D3

http://plottablejs.org/

MIT License

2.97k stars 222 forks source link

[Drawer] - Exported types are possibly a little odd #2196

Closed bluong closed 9 years ago

bluong commented 9 years ago

There are exposed types that are pretty Drawer specific and I'm unsure if they should be exposed.

Namely DrawStep and AppliedDrawStep

aicioara commented 9 years ago

They are Drawer specific, but they are used in Plots as well. For example AreaPlot._generateDrawSteps() which returns an array of Drawer.DrawSteps. The fact that it is encapsulated inside the Drawers module seems good enough.

aicioara commented 9 years ago

Won't fix. @jtlan opinion?

jtlan commented 9 years ago

DrawStep makes sense, but it's weird that AppliedDrawStep also exists.

aicioara commented 9 years ago

So the issue we face is the following.

AppliedDrawStep is used inside the protected _drawStep method only. This method is protected, but there is no way to bring a type (or interface for what matters) to this visibility level.
If someone wants to extend the class, they need to have knowledge of the AppliedDrawStep
We cannot declare the type as being non exported and then reference it to an exported method (protected). Hence, we have to explicitly type the type in the definition, which is super ugly.

Conclusion: won't fix

jtlan commented 9 years ago

Okay, sounds good to me.