BaGet icon indicating copy to clipboard operation
BaGet copied to clipboard

Published 20 hours ago • verified publisher icon loic-sharma

adding basepath support

Open szwenni opened this issue 5 years ago • 9 comments

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 https://github.com/loic-sharma/BaGet/issues/276

szwenni avatar Oct 17 '19 12:10 szwenni

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! 😊

loic-sharma avatar Oct 19 '19 17:10 loic-sharma

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.

szwenni avatar Oct 21 '19 23:10 szwenni

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.

szwenni avatar Oct 22 '19 17:10 szwenni

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.

szwenni avatar Oct 23 '19 11:10 szwenni

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.

szwenni avatar Nov 02 '19 14:11 szwenni

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.

daniel-lerch avatar Mar 25 '20 19:03 daniel-lerch

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

szwenni avatar Mar 30 '20 07:03 szwenni

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

LaplancheMaxime avatar May 06 '20 12:05 LaplancheMaxime

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?

daniel-lerch avatar May 06 '20 13:05 daniel-lerch