Skip to content

Engines are not returned to pool "per request" but rather "per Html.React method invocation" #487

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

Closed
ryedin opened this issue Dec 8, 2017 · 8 comments

Comments

@ryedin
Copy link

ryedin commented Dec 8, 2017

see: https://github.com/reactjs/React.NET/blob/master/src/React.AspNet/HtmlHelperExtensions.cs#L84

I'm wondering why the engine is being returned to the pool after each call to Html.React, when in all of the comments/docs you indicate that it should be "per request" (and I believe a true "per request" model would be much better overall performance).

Performance concerns aside, the way this is working right now also prevents using things like reactive mobx stores on the server across calls to Html.React within a given request context. We want to be able to disconnect components from the data that drives them and use dependency injection (on the js side) to connect them to the data. This prevents us from having to explicitly push ALL props in at the top level (the so-called page-level component). This works fine for the client-side rendering, as expected, because there is only a single js execution context. We had assumed that there would also be a single js execution context on the server (per request), because that would make a lot of sense. However, we soon realized that our injected data (injected via subsequent calls to Html.React in various partials) is never made available, and is thus not actually being utilized for SSR.

Are there other reasons that I am not able to see that are preventing making the engine be truly a singleton across the entire request, rather than simply per call to Html.React?

@ryedin
Copy link
Author

ryedin commented Dec 8, 2017

Got this to work as expected by disabling engine reuse in the config, but IMO, we should be able to configure pooling AND allow the same engine instance to persist for the entire request... and that should be the default behavior.

But again, please let me know if there is an explicit reason why you feel the engine(s) should be recycled per call to Html.React even if you're still in the same request.

@dustinsoftware
Copy link
Member

This behavior changed in #270.

There is discussion there - it was possible for engines not to be returned to the pool in some cases, which caused a fatal crash when all the engines are gone, so that change avoids the issue entirely.

I suppose we could make this configurable but I’m not sure there’s much value encouraging this aside from your specific use case for sharing engine scope across multiple requests, as a render call now counts on side-effects from previous render calls.

@ryedin
Copy link
Author

ryedin commented Dec 11, 2017

use case for sharing engine scope across multiple requests

You keep using the word request when you really mean method calls within the context of a single request. This isn't about sharing state across requests. It's about using the semantics of a single request to build up state as the HTML is created that will be sent back to the client. This is not uncommon, nor without value, nor should it be "not encouraged".

There is a ton of value in being able to rely on the components/modules having access to a single execution context for the lifetime of this request. It enables a more loosely coupled Component architecture, for starters, but also in the case of this project it allows you to compose the front end "app" via multiple calls to Html.React, without having to think of each call as just its own little disjointed "react app". Because we're in .NET land, and often have to work with weird frameworks that rely heavily on the Razor templating engine and partials and other HtmlHelperExtensions, this becomes invaluable. It allows us to stitch together a cohesive application without burdening us with having to create and maintain an unwieldy large set of props that all have to get pushed in at the top.

On this topic, it would also be very nice if the React components were kept "live" as the response is being constructed (using something like jsdom, or cheerio, perhaps - which is something I've done in the past with a different framework to support SSR), with a final call to the RenderAsString mechanism occurring just before the response is sent back. This will allow for interesting advanced/rich rendering scenarios where you might have things being composed across calls to Html.React that should trigger re-renders of components that were mounted earlier (which would work fine on the client of course, and is in fact a completely sound and common architecture).

I can sense, though, that this is likely to not be viewed as a priority. I'm hoping that I might have some availability to make a fork and play around with this. I assume you'd be open to a PR that supports this model, as a long as it's configurable and the defaults remain as they are now?

@dustinsoftware
Copy link
Member

Heh you are correct, I did typo earlier.

To make sure I am understanding your use case - are you suggesting to leverage the global JavaScript scope across multiple render calls as an alternative to passing bunch of props at once in to a single render call? If so, would setting props in ‘ViewBag’ via a custom html helper extension, and then referencing those props in the final render call accomplish what you are after?

Since render calls are supposed to be pure, I’m not seeing how relying on previous render calls should be a pattern this library should recommend.

Maybe a code sample or specific example would help aid this discussion along?

@dustinsoftware
Copy link
Member

(Yes, I saw the mobx example above, is there a simpler use case without a state manager that we can use for this discussion?)

@ryedin
Copy link
Author

ryedin commented Dec 11, 2017

I think we can use mobx and still keep the example simple enough. I made a gist that illustrates the basics of both the need, and the pattern.

https://gist.github.com/ryedin/6efcc8c0883bb2b1baf4d9a2f19ef105

I may have gone slightly overboard on the comments in the gist, but I wanted you to get a full picture as to what we are trying to achieve. I can't imagine this pattern is too far fetched or out of place.

Also...

Since render calls are supposed to be pure

Render calls are certainly "pure" (nothing changes that). That does not mean they cannot also be reactive (respond to changes in inputs as the larger page context is built up). Also, think in terms of how things work on the client. There is a single execution context, which is what enables these types of patterns to work. Why not think in the same terms for your SSR solution? (the "live"ness thing can be achieved with a true server-side DOM implementation, but I would be happy with just a single execution context per request for starters)

@dustinsoftware
Copy link
Member

Thanks for the gist, that was very helpful. I can see how registering data ahead of time would be useful. Longer response incoming.

@dustinsoftware
Copy link
Member

dustinsoftware commented Dec 31, 2017

Ok, so. Here's an approach that allows you to register data with Mobx ahead of any render calls, but doesn't rely on executing Javascript until a component needs to be rendered.

Registering data ahead of time from a razor file:

@Html.ReactRegisterPreloadViewData("viewName", new { a = 1, b = 2 })

React.NET would keep track of that data in a private dictionary for the scope of that request, waiting until a render call happens. You could call this helper from your existing MVC partials. Before a component SSR happens, a register function that you define is invoked, which passes any registered view data in as an argument.

import { viewDataStore } from './stores'

function RegisterPreloadViewData(viewDatas) {
  viewDataStore.init(); // remove any existing data from the current store, since JS engines get re-used, and your store could have been re-used

  for (const viewData of viewDatas) {
    viewDataStore.setViewData(viewData.viewName, viewData.data);
  }
}

Support for this doesn't exist yet, but should be pretty simple to add, and avoids issues around acquiring a single JS engine from the shared pool for the lifetime of the request.

dustinsoftware pushed a commit to dustinsoftware/React.NET that referenced this issue Aug 18, 2018
This allows UI libraries like styled-components to work without any extra code in this project

See also reactjs#487 reactjs#538
dustinsoftware pushed a commit to dustinsoftware/React.NET that referenced this issue Aug 25, 2018
This allows UI libraries like styled-components to work without any extra code in this project

See also reactjs#487 reactjs#538
dustinsoftware pushed a commit to dustinsoftware/React.NET that referenced this issue Aug 25, 2018
This allows UI libraries like styled-components to work without any extra code in this project

See also reactjs#487 reactjs#538
dustinsoftware pushed a commit to dustinsoftware/React.NET that referenced this issue Sep 26, 2018
This allows UI libraries like styled-components to work without any extra code in this project

See also reactjs#487 reactjs#538
dustinsoftware added a commit that referenced this issue Oct 2, 2018
* Add callbacks for running custom Javascript during component render

This allows UI libraries like styled-components to work without any extra code in this project

See also #487 #538

* Add babel integration test

* Add demo using styled-components

* Create abstraction for chaining pre, during, and post render callbacks

* Add dummy comments to fix build (address these later)

* Remove obsolete method reference

* Fix tests

* Cleanup some code around RenderFunctions

* Expose string property instead of closed-over func with side effects

* Fix regression in TransformRender

* Add render functions tests

* Update sample

This still needs to be better organized..

* Fix filename

* Fix build

* Update xml comments
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