Export renderer in own namespace

[lehnerchristian]

closes #208

prerequisites:

• branch is up-to-date with the branch to be merged with, i.e. develop
• build is successful
• code is cleaned up and formatted
• tested with Firefox 52, Firefox 57+, Chrome 64+, MS Edge 16+

Summary

• Remove @internal marker from Renderer, which makes them publicly available in the .d.ts files

[lehnerchristian]

@sgratzl and @thinkh : I've removed the @internal markers, however the build is failing. I don't think that this is caused by this PR or at least I'm not sure.

I found a similar problem here microsoft/TypeScript#5711 and in this issue they state that we need more explicit type annotations

can one of you fix the type annotations if they are really causing our problem because I don't really know where to start at the moment

[thinkh]

I just noticed that LineUp v3 is using an older version of the lineupengine.

LineUp v3:

lineupjs/package.json

Line 141 in 85df597

"lineupengine": "^1.1.2",

LineUp v4:

lineupjs/package.json

Line 149 in 0cfa222

"lineupengine": "^2.1.0",

I don't know the changes between lineupengine v1 and v2. Hence, as @lehnerchristian suggested, we might need to change/add some typings over there.

[sgratzl]

I just noticed that LineUp v3 is using an older version of the lineupengine.

on purpose since lineupengine v2 is designed to work with lineup v4

[thinkh]

@lehnerchristian Is this done from your side?

[lehnerchristian]

@thinkh , looks good as far as I can tell after this long time

[sgratzl]

what are the use cases again for exporting all renderers? In case of overriding them, most implement just the three factory functions, so in case one subclasses that, one doesn't gain much. Moreover, an alternative would be use just wrap one instance of an renderer with another one. Similar to the toolbar one can have access to all the registered renderers using the defaultOptions

e.g.

const base = defaultOptions().renderers.number;

...
renderers: {
 'mynumber': {
    title: 'mynumber',
    canRender: base.canRender.bind(base),
    create: base.canRender.bind(base),
    createGroup: base.canRender.bind(base),
  }
}

or

const base = defaultOptions().renderers.number;

class MyNumberR extends base.construtor {
...
}

[thinkh]

@sgratzl I understand your point that a renderer exposes only three methods and it might not be worth the opening. However, to me your suggested example looks like a different implementation pattern and unfamiliar if one is used to TypeScript where you can just import and extend a class or implement an interface. Furthermore, as a developer I would never guess that I can override or define a renderer like you suggested and would rather browse the API documentation.

@lehnerchristian and @puehringer Could you do the implementation in your app with the defaultOptions() that @sgratzl suggested? What are your opinions?

[puehringer]

@sgratzl @thinkh I indeed didn't know about these ways of renderer access/extensions. I will try to refactor the current implementation and check if it is more convenient this way.
I still do not fully get why exporting them is not a more viable solution, as we also export the corresponding models too. I understand not exporting certain helper functions, but unique classes don't seem to be much of an issue?

[lehnerchristian]

I also didn't know / think of these ways. I'll try them too
however while this const base = defaultOptions().renderers.number; gives us an instance of the renderer we need and base.constructor gives us the class itself what's the difference to exporting the renderer class itself in the first place?

[sgratzl]

Models are complex objects with various methods and business logic. Renderers are simple and since it is a flat export of lineupjs it is already getting pretty big. Moreover, when I think about it, there is no need to have proper classes for renderers after all, since they are singleton instances and usually no inheritance.

So, the more I export "officially" the less I can change in the future. like the call the base renderer in a composition style is easier than using it as a base class.

[sgratzl]

as a compromise one could expose them under a sub namespace. e.g. all the builder helper functions and classes are exported under the .builderAdapter namespace.

[thinkh]

@sgratzl Your compromise sounds like a plan. I'm not sure at the moment how to implement this. Can you please provide an example of the namespace?

[sgratzl]

e.g. as in https://github.com/lineupjs/lineupjs/blob/master/src/builder/adapter/index.ts

[puehringer]

@sgratzl @thinkh I think the exporting in a certain namespace is a good idea, as I am currently facing some issues porting apps to lineup#v4. As already discussed, some functions were annotated with @internal, causes them to be not accessible.

Some of these functions however are very essential (for advanced use-cases) such as:
Toolbar Utilities
https://github.com/lineupjs/lineupjs/blob/lineup-v4/src/ui/toolbar.ts#L362-L373
https://github.com/lineupjs/lineupjs/blob/lineup-v4/src/ui/toolbar.ts#L376-L403
CategoricalColumn
https://github.com/lineupjs/lineupjs/blob/lineup-v4/src/model/internalCategorical.ts#L146-L160
...
These are just some functions which are used in apps and cannot be easily accessed/replicated. Implementing them on our own is not just a waste of resources, but also error prone and sometimes even impossible like here.
I absolutely agree with you that we should not think export first, regret later, but certain functions such as getToolbarDialogAddons and isCategoryIncluded are essential and will most likely not change.

Do you think it would be possible to (selectively) export functions in their particular namespace?

[sgratzl]

please open another issue or lets discuss that in person. I don't want to discuss things that are not PR related in a PR since the PR will be closed when it is merged.

[puehringer]

@sgratzl For the specific exports I will create separate tickets of course, but I was just listing a few examples supporting the namespace idea discussed above.
Re: discussing things in person, what is your preferred way of contacting you? I think Github issues are good for summaries, but might not be the most efficient option.

[thinkh]

@sgratzl I reverted the @internal comments and exported the renderer classes and util functions in ./src/renderer/index.ts. Please review if this is what you meant?

[sgratzl]

yes that was the basic idea. Tho, I don't guarantee a stable API behind any namespaces. So a breaking change in a function or renderer won't lead to a major version update.

[thinkh]

@sgratzl If you are fine with the changes, you can proceed and merge this PR.