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

Resolve ASP.NET environments without HTTP contexts #449

Merged
merged 1 commit into from
Nov 11, 2017

Conversation

coka
Copy link
Contributor

@coka coka commented Oct 31, 2017

Sometimes, a user might wish to render HTML in an ASP.NET MVC web application outside of the context of a web request. An example would be email rendering in a separate thread. With the current implementations of AspNetCache and AspNetFileSystem, TinyIoC fails to inject the necessary objects because, internally, an httpContext is null.

For AspNetCache, we can obtain the cache from HttpRuntime, which HttpContextBase calls under the hood (as pointed out in the comments here).

For AspNetFileSystem, HttpServerUtilityBase is used only for .MapPath(), which HostingEnvironment provides as a static method. This method is slightly different, but that should be a non-issue. For example, if the provided relative path is null, React.Core will throw sooner anyway.

{
_cache = context.Cache;
_cache = HttpRuntime.Cache;
Copy link
Member

@Daniel15 Daniel15 Nov 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if this was injected via the dependency injection container, to more easily facilitate unit testing and decouple the components. Could you register this AssemblyRegistration.cs? Something like:

container.Register<Cache>((c, o) => HttpRuntime.Cache);

Then inject it into the constructor:

public AspNetCache(Cache cache)

@coka
Copy link
Contributor Author

coka commented Nov 10, 2017

@Daniel15 I have amended the commit to use dependency injection. The build seems flaky, because it passed on the first amend, and failed on the second one, which contained only comment / whitespace changes.

@Daniel15
Copy link
Member

Yeah, one of the tests seems flaky :/ This looks good to me!

@Daniel15 Daniel15 merged commit 2e6b9a7 into reactjs:master Nov 11, 2017
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

Successfully merging this pull request may close these issues.

None yet

2 participants