Open darcyparker opened 3 years ago
@darcyparker DOM-related operations is one of the weak points through the vscroll infrastructure. I tried to make the implementation as simple/rough as possible, relying on browser year-to-year growing performance rather than lib performance. The idea of pluggable DOM-engine is very good. Moreover, I already extracted the most part of the DOM-related logic into a separate class inside the core: vscroll/src/classes/domRoutines.ts. And I planned to make it replaceable: a consumer (vscroll-native) should be able to override this class via config of the Workflow super-entity. Currently such approach is done for the Adapter reactive props, which are configurable at the Datasource factory level: core DS factory -> consumer specific DS. The Workflow should receive domRoutines configuration.
1) Routines is not finished yet, for example items rendering is not under its control, and this relates to what you want to change: hiding items in vscroll-native is served by render process in vscroll. Both operations can be moved to Routines and Routines could be replaced by consumer. The argument of createItemElement
is an instance of Item class which has Routines instance on it. As you see, we can't separate these two operations and replace one of it (core relies on hiding!), but we can try to gather them in a single place and provide a consumer-core overriding mechanism to replace them both. This is one point of the createItemElement
problem.
2) Another one is that you want to give an option to change a consumer DOM-related logic, which is not connected with the core. This is VERY good idea, but before discussing the details, let's think about scoping. The logic we are going to deal with is the most low-level across the project, and my guess the consumer layer is the best place that matches it, so it should not leak into the end app. Let me put here the vscroll introduction image:
So your suggestion could be treated as a factory of native consumers. Setting up such a factory would require good qualifications from the end app devs. My idea is not to give in the hands of the end app devs such fundamental pieces as rendering implementation, even if it's optional. Maybe it will be better to
...
I will be glad to participate whichever way you/we choose.
My idea is not to give in the hands of the end app devs such fundamental pieces as rendering implementation, even if it's optional. Maybe it will be better to
I agree.
With regards to the 4 options:
Template
and render
can satisfy all render/template cases I can think of... and it would keep things DRY to have a the vscroll-native consumer factory.The popular choices for a renderer are likely uhtml
, lit-html
et al... and there could be more.
I am going to chew on your other comments... but also interested in helping.
@darcyparker My other comments were about the createItemElement
method can't be replaced entirely due to render-hiding-showing logic that is scattered over the core and consumer; additional efforts are needed to put it in a single configurable DOM Routines class (which is not configurable for now, and more efforts are needed to make it configurable).
But if you don't want to change these particular operations (btw they are required by the core doc), we may start the Factory developing. From the one hand, I'd be glad to see something like core Datasource factory (a parametrized function returning extended class); from the other hand, we may move toward ts abstract class or native mixins. I think, since the project is quite small, it should be not a problem to update the infrastructure. The more important is to implement a new renderer...
Once we have two different renderers (1 default and 1 new), we'll be able to generalize the Scroller entity, taking into account their specificity. The entry (and the only important) point is the Workflow run callback implementation, which is wrapped with the wf-storage:
So the updateViewport
must satisfy the requirements listed in the doc. The updateViewport
method calls createItemElement
in the end. That's how the default Scroller class is written. Another implementation of the Scroller class might not contain createItemElement
at all, it just should provide correct updateViewport
method.
Adding you to the repository collaborators for not to waste time. We may continue the discussion by the code.
@dhilt - Thanks for adding me as a repository collaborator.
I understand what you mean about the render hiding logic being scattered over core and consumer. I have some thoughts but they aren't fully formed.
I can get started with the factory development... (likely Monday).
With regards to the Datasource factory, I am open to an abstract ts class or native mixins too. But you're right, that if a factory is created, and later like another approach its probably not too hard to update. As a side question, would you be open to adding something like (index: number) => AsyncIterable<T> | (index: number) => Iterable<T>
to DatasourceGet
@darcyparker Great!
If you are speaking about a new signature for the Datasource.get
method, then why not. There are only two places where this emerges:
Also, there is a spec in ngx-ui-scroll project handling the Datasource and Fetch process running: datasource.spec.ts. I'd recommend to run it in parallel with vscroll developing, this part of the doc tells how to do it. I'd be happy to remove the Datasource spec from ngx-ui-scroll and create a new one in vscroll; it should a) explicitly check all signatures with all possible (especially wrong) arguments, b) emulate Fetch process run and check all possible immediate and delayed results.
@dhilt - FYI. I started this today and made some progress, but it is still a WIP.
vscroll
. I tried npm install --save vscroll@github:darcyparker/vscroll#scrollFactory-wip
(a temporary measure during development in my scrollFactory-wip
branch.), but for some reason the node_modules/vscroll
dir does not include the build of vscroll. I suspect this has something to do with prepack
... I am not sure why it is not running the build... Have you run into this before?scrollFactory-wip
(I expect I will rebase as my work progresses... its a work in progress as the branch name says) It's not ready for discussion/review, but you're welcome to have a look. I did backtrack on some of my previous work/experiments and expect I will do so again.vscroll
and generalize its dom methods to better fit html
and render
interfaces from libaries like uhtml
, lit-html
, https://github.com/developit/htm and the numerous others like it.@darcyparker Here's the tip on core-consumer development:
You may push your wip-branch to the original repo, open draft PR, then it may be easier to discuss
I like this vscroll-native example. But I would like to use something different than
innerHTML
in createItemElement.createItemElement
is a fine default, but there would be some advantages if something like uhtml or lithtml were an option. (I prefer uhtml, but I am sure lithtml and others could be popular too.)What if there was
ScrollerFactory
that is provided the desired renderer and returns aScroller
class with the applicableTemplate
type for the renderer chosen?Scroller.prototype.createItemElement
would be replaced with acreateItemElement<TEMPLATE>
that is provided to theScrollerFactory
and would have the same interface. But it would dictate theTEMPLATE
type for the renderer used.createItemElement
could be the same implementation as the existingScroller.prototype.createItemElement
, where theTEMPLATE
generic would be set to the existing Template type.Is this something you'd consider? If so, I am happy to see if this idea works and create a pull request. (I may have some flaws in my current thinking/idea, but my hope is that its mostly feasible and that I can work out problems if I pursue it.) I just want to proceed if the idea is not valued.
Alternatively I could use vscroll-native as an example and create my own implementation that uses my preferred renderer. But it feels like there is opportunity for reuse if there were a
ScrollerFactory
that provided the desired renderer.