Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Idea: ScrollerFactory() that allows alternate template type and renderer #2

Open
darcyparker opened this issue Sep 30, 2021 · 8 comments

Comments

@darcyparker
Copy link
Collaborator

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 a Scroller class with the applicable Template type for the renderer chosen?

  • The existing Scroller.prototype.createItemElement would be replaced with a createItemElement<TEMPLATE> that is provided to the ScrollerFactory and would have the same interface. But it would dictate the TEMPLATE type for the renderer used.
  • As a default, the provided createItemElement could be the same implementation as the existing Scroller.prototype.createItemElement, where the TEMPLATE 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.

@dhilt
Copy link
Owner

dhilt commented Sep 30, 2021

@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

  • implement the best approach and replace the existed one inside vscroll-native
  • develop all rendering options inside one (vscroll-native) consumer and provide simple switching setting
  • turn vscroll-native into a consumer factory, but publish each stand-alone implementation as a separate library with no DOM-config (1 extra dependency)
  • forget about vscroll-native and develop completely separate vscroll-consumer with its own unique rendering (no extra dependency)
  • ...

I will be glad to participate whichever way you/we choose.

@darcyparker
Copy link
Collaborator Author

darcyparker commented Sep 30, 2021

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:

  • The 'best approach' is subjective... and could change
  • developing all rendering options inside one consumer seems overkill
  • I like the idea of turning vscroll-native into a consumer factor and publish each stand alone implemention.
  • I also like the idea of forgetting about vscroll-native... but I like the previous choice because I think the interface for a 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.

@darcyparker
Copy link
Collaborator Author

I am going to chew on your other comments... but also interested in helping.

@dhilt
Copy link
Owner

dhilt commented Oct 1, 2021

@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:

onItemsChanged: this.updateViewport.bind(this)

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.

@darcyparker
Copy link
Collaborator Author

@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? (Notice a count is not necessary because the consumer can iterate as much as needed.)

@dhilt
Copy link
Owner

dhilt commented Oct 2, 2021

@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.

@darcyparker
Copy link
Collaborator Author

darcyparker commented Oct 5, 2021

@dhilt - FYI. I started this today and made some progress, but it is still a WIP.

  • Export worflow params vscroll#28 (minor pull request)
  • I ran into some issues that will likely require some experimentation/tweaking of 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?
  • In my fork, I have a 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.
  • I am going to chip away at this as time permits... I think its going to take a bit more time to dive into 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.

@dhilt
Copy link
Owner

dhilt commented Oct 5, 2021

@darcyparker Here's the tip on core-consumer development:

  • clone both vscroll and vscroll-native repositories into the same folder
  • replace "vscroll" import with local sources here

You may push your wip-branch to the original repo, open draft PR, then it may be easier to discuss

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants