adding basepath support by szwenni · Pull Request #388 · loic-sharma/BaGet

szwenni · 2019-10-17T12:54:53Z

Summary of the changes (in less than 80 chars)

Adding full PathBase support
Adding dynamic PathBase replacing in react scripts on StartUp of server

Addresses #276

loic-sharma · 2019-10-19T16:58:28Z

src/BaGet.Core.Server/Configuration/ConfigureForwardedHeadersOptions.cs

 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.HttpOverrides;
 using Microsoft.Extensions.Options;
+using System;


Please place put using System* first. Visual Studio can do this automatically if you enable the option Place 'System' directives when sorting usings under Text Editor > C# > Advanced.

Okay i will change that in next commit. I wasn't aware of that option.

loic-sharma · 2019-10-19T17:01:22Z

src/BaGet.Core.Server/Configuration/ConfigureForwardedHeadersOptions.cs

    public class ConfigureForwardedHeadersOptions : IConfigureOptions<ForwardedHeadersOptions>
    {
-        public void Configure(ForwardedHeadersOptions options)
+        private BaGetOptions bagetOptions;


A few suggestions:

Please add the readonly field modifier to ensure no one accidentally modifies this value.

Please keep the options wrapped in a IOptionsSnapshot to support automatic configuration reload.

Please prefix field names with an underscore so that you don't need to use this. to refer to fields.

Suggested change

private BaGetOptions bagetOptions;

private readonly IOptionsSnapshot<BaGetOptions> _options;

I will update this also in next commit. I wasn't aware of the IOptionsSnapshot thing if I'm honest, but it makes sense.

I couln't get it to work with IOptionsSnapshot because BaGetOptions and ConfigureForwardedHeadersOptions are in different scopes. Maybe you can get it to work.

loic-sharma · 2019-10-19T17:05:07Z

src/BaGet.Core.Server/Configuration/ConfigureForwardedHeadersOptions.cs

+                Console.WriteLine("Host Url: " + bagetOptions.HostUrl);
+                options.ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto | ForwardedHeaders.XForwardedHost;
+                options.AllowedHosts = new List<string>() { bagetOptions.HostUrl};
+            }else


Please add new lines in between blocks:

if (...) { ... } else { .. } // Do not restrict to local network/proxy

loic-sharma · 2019-10-19T17:07:07Z

src/BaGet.Core.Server/Configuration/ConfigureForwardedHeadersOptions.cs

+        {
+            if (!string.IsNullOrEmpty(bagetOptions.HostUrl))
+            {
+                Console.WriteLine("Host Url: " + bagetOptions.HostUrl);


Can we remove this? Or do you think this will be useful for customers? If the latter, please log using an ILogger<ConfigureForwardedHeaderOptions> as not everyone has access to stdout to browse logs

Mabe leaving it hast Debug message should be fine. If somehow problems occur you could enable debug and this might help.

I will take the logger framework. I wanted to change this before pull request but forgot it.

loic-sharma · 2019-10-19T17:08:10Z

src/BaGet.Core/Configuration/BaGetOptions.cs

        public string PathBase { get; set; }

+        /// <summary>
+        /// The application Host without port number. If not set all hosts will be allowed. See https://github.com/aspnet/Announcements/issues/295


Nice link! 👍

Found it in your comments and it makes sense to explain why this is required.

loic-sharma · 2019-10-19T17:28:52Z

src/BaGet.UI/src/index.tsx


 import DisplayPackage from './DisplayPackage/DisplayPackage';
 import Upload from './Upload';
+import createBrowserHistory from 'history/createBrowserHistory';


What's this for?

Forgot to remove it.
I tried adding the history to react router and setting the base path via the history.
But dom router does not support history so I had to use the basename option.

loic-sharma · 2019-10-19T17:56:51Z

Hi, this is awesome, thank you for opening this change! I wasn't sure how to share configs between the backend and the frontend, this is a really neat solution! I'm wondering, how did you choose to override the frontend configs when the app middleware is created in UsePathBase?

I wonder if instead we should upgrade BaGet's AddSpaStaticFiles. AddSpaStaticFiles registers an ISpaStaticFileProvider, which exposes an IFileProvider (that IFileProvider is actually a PhysicalFileProvider). We could create our own ISpaStaticFileProvider that has a custom IFileProvider that replaces configurations lazily: it would return the IFileInfo from the PhysicalFileProvider if we detect no configs, otherwise we could create an in-memory IFileInfo with the contents replaced. This has the added benefit that we don't mess with our UI's build artifacts. What do you think of that approach?

Thanks again for opening this, I really appreciate it! 😊

szwenni · 2019-10-21T23:05:50Z

Hi, thnk you very much.
Using UseBasePath was the most straight foreward approach for that.
I didn't dig to much into this but your approach seems to be a nicer way to do this.

So basically from the approach I took we have to always place our configuration in the UI files because I choosed a placeholder which will be replaced. So I would suggest to always have the files in-memory. From performance side there should be little to no impact.
I think implementing shouldn't be that hard. I will try to make the required changes.

Uses in memory for all js files and index.html Some code cleanup is still neccessary but it works frm first testings For all other files that are not js files and not the index html PhysicalFileInfo is taken as fallback

szwenni · 2019-10-22T17:54:37Z

So
I made the required changes to get it to work with ISpaStaticFileProvider.

Take a look at it and tell me your thoughts. I think its a quite neat way to have this in-memory but still with reasonable performance.

Fixed Logger null error

szwenni · 2019-10-23T11:52:04Z

So with the latest commit tests are working again.

Still code cleanup needs to be done.

Also we should consinder to write some tests for the StaticFileProviding.

If you have time you can have a look at the implementation and tell me your findings.

src/BaGet.Core.Server/Configuration/ConfigureForwardedHeadersOptions.cs

loic-sharma · 2019-10-28T14:54:03Z

src/BaGet/Startup.cs

                if (env.IsDevelopment())
                {
-                    spa.UseReactDevelopmentServer(npmScript: "start");
+                    //spa.UseReactDevelopmentServer(npmScript: "start");


Do we need to support a custom host name while in development mode? That seems like a weird use case to me. Would it be better if we didn't support a custom host name while in development mode? The React development server is very useful :)

Yeah you are right. I don't know enough about the react environment. But maybe it is possible to have like dev environment where homepage is set to "" and a prodution environment where homepage is set to __BAGET_PATH_BASE_PLACEHOLDER__. So that npm (react) build setups the runtime and the index.html relative paths correctly.

I will try to find a appropiate solution for that.

…ptions.cs Co-Authored-By: Loïc Sharma <sharma.loic@gmail.com>

…ly in production mode commented reactdevserver back in whoch will be used in dev mode

…to basePathSupport

szwenni · 2019-11-02T14:21:14Z

The Placeholder for path base will be now set only in production mode of npm (npm run build), if you use npm start no relative path will be appended to the start.

daniel-lerch · 2020-03-25T19:12:17Z

Which changes have to be made for this PR to get merged?
I have to use BaGet with a custom PathBase and implemented it for the backend in #114.
Please let me know if there is anything that I can do to complete this feature.

szwenni · 2020-03-30T07:47:21Z

I don't know you need to ask @loic-sharma

Summary of the changes * `UsePathBase` only works if applied before any file servers * This is just a quick fix before #388 is merged

LaplancheMaxime · 2020-05-06T12:09:31Z

Hi, I need this feature. Do you need help completing it?
Have a nice day

daniel-lerch · 2020-05-06T13:13:25Z

I just took a look into the code and there is a lot of cleanup work that needs to be done. Some classes look more like Java code than C# and ASP.NET has a lot of patterns which the rest of BaGet is following.

Instead of writing dozens of review comments it would be easier for me to fix stylistic mistakes directly.
@szwenni Do you want to give me push access to your fork or shall I open a new pull request?

Sven Kröger added 2 commits October 17, 2019 14:38

adding basepath support

4c93b1d

adding check if ui folder exists

b5c54fe

loic-sharma reviewed Oct 19, 2019

View reviewed changes

Sven Kröger added 2 commits October 22, 2019 19:43

Implemented StaticFileProvider for SPA

1d5fd6c

Uses in memory for all js files and index.html Some code cleanup is still neccessary but it works frm first testings For all other files that are not js files and not the index html PhysicalFileInfo is taken as fallback

removed history from index.tsx

88b97ab

Sven Kröger added 2 commits October 23, 2019 12:15

Adding better development support

14ce74a

Fixed Logger null error

logger null fixed again

966de13

loic-sharma reviewed Oct 28, 2019

View reviewed changes

src/BaGet.Core.Server/Configuration/ConfigureForwardedHeadersOptions.cs Outdated Show resolved Hide resolved

loic-sharma reviewed Oct 28, 2019

View reviewed changes

szwenni and others added 3 commits November 2, 2019 14:55

Update src/BaGet.Core.Server/Configuration/ConfigureForwardedHeadersO…

b37b367

…ptions.cs Co-Authored-By: Loïc Sharma <sharma.loic@gmail.com>

added production environment variables for setting the placeholder on…

405ada5

…ly in production mode commented reactdevserver back in whoch will be used in dev mode

Merge branch 'basePathSupport' of https://github.com/szwenni/BaGet in…

0ca9def

…to basePathSupport

daniel-lerch mentioned this pull request Mar 25, 2020

Fix request pipeline for custom PathBase settings #495

Merged

loic-sharma pushed a commit that referenced this pull request May 3, 2020

Fix request pipeline for custom PathBase settings (#495)

4aa6b21

Summary of the changes * `UsePathBase` only works if applied before any file servers * This is just a quick fix before #388 is merged

daniel-lerch mentioned this pull request May 7, 2020

PathBase support for React UI #511

Closed

	private BaGetOptions bagetOptions;
	private readonly IOptionsSnapshot<BaGetOptions> _options;

Conversation

szwenni commented Oct 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma Oct 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loic-sharma commented Oct 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szwenni commented Oct 21, 2019

Uh oh!

szwenni commented Oct 22, 2019

Uh oh!

szwenni commented Oct 23, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szwenni commented Nov 2, 2019

Uh oh!

daniel-lerch commented Mar 25, 2020

Uh oh!

szwenni commented Mar 30, 2020

Uh oh!

LaplancheMaxime commented May 6, 2020

Uh oh!

daniel-lerch commented May 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

loic-sharma Oct 19, 2019 •

edited

Loading

loic-sharma commented Oct 19, 2019 •

edited

Loading