Skip to content

Conversation

aldrashan
Copy link
Contributor

@aldrashan aldrashan commented Jul 20, 2020

Couldn't compile with all the <Compile Include="..\SharedAssemblyVersionInfo.cs" /> lines, since those files were missing, so had to remove those to test.

Relevant changes are in the src/React.AspNet/HtmlHelperExtensions.cs file.

In short: the best solution to implement these changes would be to have access to an IUrlHelper instance.
Since we call @Html.ReactGetScriptPaths() inside a view, we can change this to @Html.ReactGetScriptPaths(Url).
This way we can have access to the IUrlHelper.Content(...) method and we can use a relative path inside our webpack.config:

new ManifestPlugin({
   fileName: "asset-manifest.json",
   publicPath: "~/dist/", <------------------------------------------
   generate: (seed, files) => {
      const manifestFiles = files.reduce((manifest, file) => {
         manifest[file.name] = file.path;
         return manifest;
      }, seed);

      const entrypointFiles = files.filter(x => x.isInitial && !x.name.endsWith('.map')).map(x => x.path);

      return {
         files: manifestFiles,
         entrypoints: entrypointFiles,
      };
   }
})

The generated asset-manifest.json file then looks like this:

{
  "files": {
    "main.css": "~/dist/main.styles.css",
    "main.js": "~/dist/main.80e487ae.js",
    "runtime.js": "~/dist/runtime.10bceee9.js",
    "vendor.js": "~/dist/vendor.8faee7f5.js"
  },
  "entrypoints": [
    "~/dist/main.styles.css",
    "~/dist/main.80e487ae.js",
    "~/dist/runtime.10bceee9.js",
    "~/dist/vendor.8faee7f5.js"
  ]
}

This then gets correctly translated when your website isn't hosted as root (e.g. as a subapplication inside iis).

Successfully tested on a core 3.1 webapplication.

@dustinsoftware
Copy link
Member

dustinsoftware commented Jul 22, 2020

Hmm, did running dev-build.bat in the project root not create that file for you?

Should be able to look at this soon, thanks for the PR.

@dustinsoftware dustinsoftware changed the base branch from master to main July 23, 2020 04:21
@aldrashan
Copy link
Contributor Author

Undid all my changes, ran dev-build.bat and added my changes again.
Works for netstandard2.0, netcoreapp3.0 and React.Web.Mvc4 now.
Test and checks have passed also now.

@dustinsoftware
Copy link
Member

dustinsoftware commented Aug 4, 2020 via email

@arrudamarcos78
Copy link

Thanks for much for sending the PR, I really appreciate you taking the time to do so. I did a bit of digging and the library doesn't really support IIS virtual applications, although arguably it should, since that is a reasonably common use case for enterprise systems. This PR is a pretty low level way to accomplish that by opting out of UrlHelper, I would prefer to have that set in ReactConfiguration instead and have the URL paths be written relative to the application root. I'm guessing the current root can't be detected automatically so we may need to make that explicit via the configuration. So as-is I don't want to merge this yet unless we have a more fully-featured story for how IIS virtual applications work :) In theory, if an option was added to ReactConfiguration that looked something like public string ScriptRoot {get; set;}, where ScriptRoot is either [https://example.com/some-virtual-app-name](https://example.com/some-virtual-app-name%60) or /some-virtual-app-name, then asset URLs could be written using that as a base.

On Sun, Aug 2, 2020 at 10:57 PM, aldrashan @.***> wrote: Undid all my changes, ran dev-build.bat and added my changes again. Works for netstandard2.0, netcoreapp3.0 and React.Web.Mvc4 now. Test and checks have passed also now. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1141 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHGCFQKIMT2VE4M23FTWV3R6ZGVHANCNFSM4PCPIXDA .

Ok, but how can I have access to this alternative solution shown above, since it's not available as an official version in NuGet? I really don't want all the trouble of having to download the project and compile it for myself... could you please send me a compiled version of React.AspNet.dll containing these changes?

@dustinsoftware
Copy link
Member

dustinsoftware commented Aug 18, 2020 via email

@aldrashan
Copy link
Contributor Author

Thanks for much for sending the PR, I really appreciate you taking the time to do so. I did a bit of digging and the library doesn't really support IIS virtual applications, although arguably it should, since that is a reasonably common use case for enterprise systems. This PR is a pretty low level way to accomplish that by opting out of UrlHelper, I would prefer to have that set in ReactConfiguration instead and have the URL paths be written relative to the application root. I'm guessing the current root can't be detected automatically so we may need to make that explicit via the configuration. So as-is I don't want to merge this yet unless we have a more fully-featured story for how IIS virtual applications work :) In theory, if an option was added to ReactConfiguration that looked something like public string ScriptRoot {get; set;}, where ScriptRoot is either [https://example.com/some-virtual-app-name](https://example.com/some-virtual-app-name%60) or /some-virtual-app-name, then asset URLs could be written using that as a base.

If you don't mind me asking, what's the reason you don't want to use an (I)UrlHelper instance to generate the paths?
I've only ever hosted applications with IIS, so I'm curious to know if it somehow works differently in Linux or something.
I can't see any downsides to using it. Granted, having to pass the (I)UrlHelper instance, like in this PR, isn't ideal, but I've changed my local version to fetch one via the passed IHtmlHelper instance and that works as well, so people wouldn't have to change anything in order to activate the new functionality.

The code looks like this:

internal static IUrlHelper GetUrlHelper(this IHtmlHelper htmlHelper)
{
    try
    {
        #if LEGACYASPNET
             return new IUrlHelper(htmlHelper.ViewContext.RequestContext, htmlHelper.RouteCollection);
        #else
	    var urlHelperFactory = (Microsoft.AspNetCore.Mvc.Routing.IUrlHelperFactory)htmlHelper.ViewContext.HttpContext.RequestServices.GetService(typeof(Microsoft.AspNetCore.Mvc.Routing.IUrlHelperFactory));
	    var urlHelper = urlHelperFactory.GetUrlHelper(htmlHelper.ViewContext);
	    return urlHelper;
        #endif
    }
    catch (Exception)
    {
        return null;
    }
}

I can push that version if you want to take a look at it.

Copy link
Member

@dustinsoftware dustinsoftware left a comment

Choose a reason for hiding this comment

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

After considering this for a while I’ve come around to merging it as is. If we change the API in the future we can always provide a deprecation notice in advance.

@dustinsoftware dustinsoftware merged commit 4e62e3c into reactjs:main Aug 26, 2020
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.

4 participants