-
Notifications
You must be signed in to change notification settings - Fork 41.5k
Add reactive progressive rendering features to MustacheView #16829
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't this be List<String>
which at the end in getBuffers
becomes Flux.fromIterable(this.buffers).map(...)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think that would work, would it? How would you concatMap
another Publisher
onto it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see the write(Object)
method which handles Publisher values. Still isn't it all collected/aggregated before response#writeAndFlushWith
is called? So it could be a List<Object>
(either String or Publisher) which can then be handled through a combination of Flux.fromIterable
and concatWith
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there’s an advantage to doing it that way, I suppose so. What would be the point - you’d end up having to iterate over the list and do the same at the end anyway, wouldn’t you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For efficiency I guess, accumulating vs composing with a new operator for every write. If there are a lot of writes, this could end up with a lot of unecessary objects and an unnecessarily long pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn’t reactor optimize that away? Could it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a list-based refactoring: 3c4fc78. Is that what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a further simplification rstoyanchev@fc5a2cb.
@bclozel I remember some talk about this on the Framework call, is the plan to move the code?
We did discuss this idea but since all the variants are already in Spring Boot (and there’s no benefit in moving those right now) we chose to let them here for now. In general Framework is implementing a couple of views just for reference implementation but would rather see those in third party projects where they can evolve more (like Thymeleaf).
I’ll review this PR and add more tests, especially for memory leaks.
35a609d
to
38d8ccb
Compare
One comment I have is, the wrapping of Flux/Mono/Publisher attributes with FluxLambda
suppresses the default behavior of AbstractView
to resolve Mono and Flux to concrete values. I think the streaming should be something you opt into for certain attributes. The default assumption should still be to resolve asynchronous values prior to rendering.
In addition, the way I thought of progressive rendering from spring-projects/spring-framework#19547 is that a basic wireframe of the complete HTML page would render, leaving multiple fragments to be completed with asynchronous data from a Flux blended with markup. What I see here is sequential rendering where multiple Flux-based fragments would be rendered one at a time too.
Maybe you missed the documentation ? User has to opt into progressive rendering with a naming convention on the model attribute. I’m happy to argue or yield on what the convention should be, but that seemed the most straightforward and pragmatic approach to me.
The sequential aspect I leave to you. For me what we have here works for the cases that are supported in thymeleaf (and is easier for users), so I was happy to leave it at that. AFAIK thymeleaf only supports one reactive model element per view, which seems limiting to me. But I didn’t go very deep in what happens when multiple attributes are provided here: it works but was only tested with a single attribute.
I'm not sure, but I was under the impression the only way to get multiple fragments streaming at once, or to embed streaming content inside a completely rendered "wireframe", is to use separate SSE endpoints and some JavaScript on the main view. That's the way the Thymeleaf demo works, and it's the same for this proposed feature with Mustache.
User has to opt into progressive rendering with a naming convention on the model attribute.
Indeed I missed the docs, sorry.
The Spring Framework also has conventions for such attributes when added to the model without a name. It looks like currently it is fluxSomething
vs somethingFlux
which is a bit too easy to forget which is which. Perhaps all we need is just one prefix, like what Thymeleaf does, just to signal a streaming attribute (e.g. stream:something
)? It doesn't matter if it is Flux or Publisher.
For me what we have here works for the cases that are supported in thymeleaf (and is easier for users), so I was happy to leave it at that.
Indeed it's a place to start
I didn’t go very deep in what happens when multiple attributes are provided here: it works but was only tested with a single attribute.
I would expect each Flux/fragment to be rendered sequentially.
I'm not sure, but I was under the impression the only way to get multiple fragments streaming at once, or to embed streaming content inside a completely rendered "wireframe", is to use separate SSE endpoints and some JavaScript on the main view.
IIRC there were multiple options. One option was to render a few HTML segments initially, e.g. <div id="section1">...</div>
, <div id="section2">...</div>
, etc, and then each fragment streamed from the server is matched to the corresponding section by id via JavaScript, which also implies that multiple Publishers are flatmapped rather than concatenated so each section can emit data fragments as they become available.
One correction. The link above to what Thymeleaf does is actually an example of how dialects can contribute async attributes to be resolved prior to rendering (e.g. Publisher but also Supplier and Function). For the streaming scenario, it's not a naming convention but rather a wrapper class.
Yes, the wrapper really didn’t appeal to me. A model is a model, it shouldn’t need special object types that tie it to the view layer.
I'm not really getting what you mean with the fragments and flat maps. It would mean the template has JavaScript in it, right? Would the fragments be rendered at the bottom of the page into a script? Do you have an example? I'm not sure why that wouldn't work with the current implementation, but it's hard without a concrete example.
Yes, the wrapper really didn’t appeal to me.
I didn't mean to suggest that. Still the problem remains.
I'm not sure I understand what the problem is. There's already a naming convention somewhere and we should try and match it (not clear why that would be)? The test case you linked to didn't really explain to me either the origin or intent of the convention.
The default naming convention for attributes added to the model is "something[Flux|Mono]"
while this view looks for attributes named "[flux|mono]Something"
. The former are resolved to concrete values, while the latter are not. I am just saying this too easy to mix up.
The former are resolved to concrete values because they don't match a convention, however that convention is too close syntactically to the one used by Spring WebFlux already when adding model attributes with no name. But you are not saying that a naming convention is a bad idea? So what would work? Maybe a name that suggests that the rendering will be different (unresolved.*
or something)?
Or async.
or maybe async:
.
I added support for both "async." and "async:" in the latest commit.
Are you planning to pic up these rstoyanchev@fc5a2cb?
I wasn't aware of those changes. Can you send a PR to my branch?
Done
9279af3
to
a26158c
Compare
a26158c
to
e14913f
Compare
fb8ad7e
to
05ad955
Compare
a326fff
to
fab80d6
Compare
1ca278f
to
902dd0b
Compare
Based on a naming convention, Model attributes of reactive types can be
progressively rendered. If the media type is SSE then an additional
Lambda is available for prepending
data:
to output lines.