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

Create a mechanism in Flow that allows creating elements in the client-side memory from server-side without attaching them to the DOM #2826

Closed
gilberto-torrezan opened this issue Nov 1, 2017 · 14 comments

Comments

@gilberto-torrezan
Copy link
Contributor

This is related to the current implementation of ComponentRenderers introduced in #2720.

The problem:
Currently the each component generated by a ComponentRenderer is created at the server-side and attached to a specific container at the client-side. And a special webcomponent, called <flow-component-renderer>, which stamped via a <template>, knows how to fetch items from this specific container and place the components at the right place.

The problem is that this container is a node inside the DOM tree, that can be manipulated and even removed by any other script in the page, making it potentially unstable. It also pollutes the client-side tree just to provide a way to the webcomponent to fetch the right items.

Example (after the <template> is stamped):

<div id="123-456">
    <my-custom-component itemkey="1"></my-custom-component>
</div>
<!-- ... -->
<vaadin-grid-cell-content>
    <flow-component-renderer container="123-456" itemkey="1"></flow-component-renderer>
</vaadin-grid-cell-content>

The internal implementation of the <flow-component-renderer> makes it look for the container with id 123-456 and fetch the component with the attribute itemkey="1" and add it as its child element.

A possible solution:
A better approach would be to create the element at the server side and store it in the memory of the client-side engine, without attaching it to the DOM. A function would be then exported by the client engine to enable the webcomponent to fetch the desired element without consulting the DOM tree.

That would make it easier to cleanup the state as well, once the components are not needed anymore.

There's no clear way on how to do this with Flow currently. There's no documentation on how the client engine operates and how the elements from server to client are bound together. So the idea is to provide a protocol to make this kind of implementation to work without the container workaround. Ideally, all the Element API for the rendered component should work out-of-the-box, so all the features for Components would just work without extra effort from the end user.

@denis-anisimov
Copy link
Contributor

I think we can do this via the same way as attachedElementById is done.
It's not a part of some generic functionality but rather a hardcoded usecase.

Also we can decide to postpone this until server-side/client-side communication refactoring is done (which we would like to do and it's theoretically should be done at some point).
May be it will give a simpler way to do this.

@Legioth
Copy link
Member

Legioth commented Nov 17, 2017

Doesn't attachedElementById rely on doing an additional round trip to the server for finalizing the operation? That would be especially unfortunate in this case that is already quite performance sensitive (i.e. #2935).

@denis-anisimov
Copy link
Contributor

denis-anisimov commented Nov 17, 2017

Yes, attachedElementById does specific checks with server-side roundtrip.
But I referred to it as an example to make some special call on the client-side.

Technically we can execute JS from the server side at any point.
The problem is that sometimes this JS requires access to the client engine code (I think that the case). And we need a special JS function available on the client side as an entry point to the engine calls.

That's done with attachedElementById: there is just a JS call on the server side. And the function is implemented on the client-side. Then it's just about the client-side implementation.

If there shouldn't be a response to the server then just don't do it in this impl.

@denis-anisimov
Copy link
Contributor

I'm not entirely sure that I understand the requirements in the ticket.
So let me ask a number of questions:

  • As I understand from the example above you would like to create an element <div id="123-456"> on the client side without attaching it to the DOM , right ?
  • As I see from the code the container element is created as an Element on the server side and attached to the DOM.
  • But you don't have to create the element via the server side. You can execute JS which creates the element on the client side only. It's possible already now.
  • The question is : how you are going to address this element if it's not attached to the DOM? You won't be able to query document since the element is not attached.
  • Now the further question is about the content of the element. In the example above it contains custom component(s) child(ren). Do you want to have the functionality which adds children into this in-memory element on the server side ? Because again: on the client side you can call JS which will fill add any children to the element. And it's possible to do already now.

So technically I don't see why you need any additional functionality from the engine. It's possible to do everything via JS already now.
Just call either JS code from the server side or "attach" JS code (file) to the component in the same way as it's done for grid connector.

But that won't be possible if you want to add/append server-side components into the pure-client side element.

So please clarify the usecases/requirements.

@denis-anisimov
Copy link
Contributor

And by the way : do you need the container as a DOM element?
It looks like you need a way to get it's children only.
So if it's a JS object which gives a way to get a list of DOM elements it should be enough. No need to create a DOM element at all.

@Legioth
Copy link
Member

Legioth commented Nov 24, 2017

This is directly based from the needs or the component renderer:

  • On the server, it should be possible to attach a regular Element instance to the state node of a <vaadin-grid> (or any other element of course) so that the element's state node will be sent to the client.
  • On the client, the element should be created and configured in the same way as a "normal" element, with the only difference being that Flow should not append it as a child of any other element.
  • Instead, attaching the element is done by JS that runs asynchronously in the component renderer's custom element implementation. The custom element instance only knows the state node id of the element it should append as a child of itself when it's attached, so there needs to be a globally exported JS API that returns the element instance for a given id.

Compared to the current implementation, this would help us get rid of the dummy container element to which the elements are currently temporarily attached.

@denis-anisimov
Copy link
Contributor

So the main need is : have an Element on the server side and its StateNode counterpart on the client side so that its DOM Node is not attached to the document. Everything else should work in the same way as for regular elements except that there should be JS API which allows to get the DOM Node by the server side id, right ?

Several observations:

  • StateNode has to be attached otherwise it's not addressed (there is no id) on the client side.
  • It means that we will need a special NodeFeature which behaves in the same way as ElementChildrenList except that the created DOM Node shouldn't be attached as a child to the client-side element. All other aspects of this feature should work in the same way.
  • It means actually that we don't need to do anything via JS in addition to the feature semantic.
  • One thing should be done though via JS: expose JS function which allows get the container element by the id.

What could be done theoretically:

  • Create an Element and add it as a child as it's done now.
  • Call JS which detaches this element from the DOM on the client side and makes it possible to get it via the id.

The only issue here is: the server side element is still the child of its parent and it's not removed.
And it can't be removed before JS is called because JS is always called in the very end.
So it can be removed only after the client-side roundtrip.

It would be good to avoid creating a dedicated feature for this usecase. And do everything via JS only. But I don't see how it can be done because we need the node attached which means it should be in some feature to be sent to the client.

So technically it's just about creating one more feature which is handled almost in the same way as ElementChildrenList plus JS API which allows to get DOM Node by the id.

@Legioth
Copy link
Member

Legioth commented Nov 24, 2017

Call JS which detaches this element from the DOM on the client side and makes it possible to get it via the id.

The entire point of this ticket is to get rid of the runtime overhead of attaching the element to the client-side DOM multiple times.

It might be possible to reuse the existing VirtualChildrenList namespace, but otherwise it would probably be necessary introduce yet another namespace for this functionality.

@denis-anisimov
Copy link
Contributor

Yeah, I've been thinking about this.
The client-side semantic of VirtualChildrenList namespace might be not adaptable.
But let's see.

@Legioth
Copy link
Member

Legioth commented Nov 24, 2017

Could maybe also be possible to slightly modify the current users of VirtualChildrenList to adapt to any changes that are necessary for making it support this case.

@denis-anisimov
Copy link
Contributor

To be able to fix this ticket and reimplement two features (which we are currently experiencing problems with) I suggest:

  • Do not use JS for this kind of features at all. Use a dedicated namespaces (NodeFeatures) generally to handle various usecases properly.
  • So it will require a dedicated logic on the client side engine to handle those usecase specifically.
  • Use only one NodeFeature for this. It will allow to reduce a number of features overall, combine all "virtual children" concept in one place on the server side and use only one NodeFeature for those children.
  • Reuse VirtualChildrenList NodeFeature for this purpose.

But VirtualChildrenList should be modified. We still need to distinguish a number of usecases.
So we need namespaces inside one namespace. The top level namespace is VirtualChildrenList.
To be able to distinguish various usecases of such virtual children we should introduce namespaces inside VirtualChildrenList.

I suggest to use JsonObject as a value in the VirtualChildrenList ( extends NodeList<JsonObject>).
The JsonObject instance consist of:

  • "type" property. This is the id of subnamespace. (e.g. "@id" for injecting by Id)
  • "node" the subject StateNode instance .
  • "payload" (or "value") : additional information (external for the StateNode above) needed for the feature. Normally we need it as a String but can have any type. For injecting by Id it's the "id" attribute value (though in this specific case it's not required as an external info because it's a part of the StateNode) . For template-in-template feature it's path of indices (address of the template element that we use to identify it on the client-side).

Generally on the client side the virtual children should be handled in the same way from the point of view of structure. The only difference is : mapping the DOM node and the StateNode instance.

@denis-anisimov
Copy link
Contributor

We should not modify VirtualChildrenList right now since its logic is used for the existing unrelated feature.
So let's introduce another NodeFeature to make a fix for this ticket and then reuse it for the two mentioned features. Then remove VirtualChildrenList and rename the introduces feature to VirtualChildrenList .

@denis-anisimov
Copy link
Contributor

OK, using JsonValue as values in the NodeMap is complicated.
It's not supported out of the box (having StateNode as a property value).

So it looks like it would be better to create a fake interim StateNode with some specific features
which will contain the real StateNode as a child + required data in other features.
This won't require modifications on the server-side logic except introducing new features.

@denis-anisimov
Copy link
Contributor

See also #3078 for JS API.

@pleku pleku closed this as completed Dec 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants