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

Correctly generate cdn base url for local scripts #8042

Merged
merged 6 commits into from
Dec 23, 2020

Conversation

deanmarcussen
Copy link
Member

Fixes #7990

@agriffard agriffard requested a review from jtkech December 17, 2020 15:10
// The application path is empty if it is the host site
// or contains the base path of a tenant.
[Theory]
[InlineData("")]
Copy link
Member Author

Choose a reason for hiding this comment

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

@jtkech i added the correct values for the application path here (base path)
but while I am in this piece of code what is the correct value for a virtual path?
and I will add it as well.

leasing slash? Or without?

Copy link
Member

@jtkech jtkech Dec 17, 2020

Choose a reason for hiding this comment

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

/virtual/tenant

When mapping to a virtual path

PathBase = /virtual
Path = /tenant/...

Then in our middleware we set

PathBase = /virtual/tenant
Path = /...

Copy link
Member Author

Choose a reason for hiding this comment

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

@jtkech I updated this to add a test for the WithBasePath / SetBasePath

I think that feature, despite having some notation of being a virtualPath is disconnected from what we would actually call a virtualPath (i.e. under IIS).

Not sure how useful the WithBasePath actually is, but it's there, and works.

Copy link
Member

Choose a reason for hiding this comment

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

@deanmarcussen

That's okay it's just a question of semantics, as you want ;)

About the base path i was thinking about the httpRequest PathBase that may contain a virtual path (i.e. under IIS) and or a tenant prefix, the tenant prefix being a kind of virtual path, but at the end the whole being a base path.

So forget the virtual term, just use basePath, and yes better to have

ResourceDefinition SetBasePath(string basePath)

That said in the tests, for me /tenant and /virtual/tenant are not applicationPath, but basePath, and then in /tenant/scripts/script.js, /scripts/script.js is a path


Okay just saw in the ResourceDefinition code the usage of GetTagBuilder(..., string applicationPath, ...) which is called somewhere by GetHtmlContent(string appPath) which is called by GetHtmlContent(_options.ContentBasePath), and where _options.ContentBasePath is initialized with the httpRequest PathBase ;)

        options.ContentBasePath = _httpContextAccessor.HttpContext.Request.PathBase.Value;

So okay, we could say that applicationPath is the same as basePath, but here i prefer basePath, at least tenantPath as an application can have multiple tenants, but each one being a kind of an isolated app ;)


https://base.com is not a basePath, nor an applicationPath, but a full Uri (Url is okay), and e.g. in the full uri https://base.com/virtual/tenant/scripts/script.js, /virtual/tenant is a basePath and /scripts/script.js is a path

Maybe good to have a test with /virtual/tenant, just to do a test with a basePath having 2 segments

But as said it's just a question of terminology, so as you want ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I agree @jtkech .

The terminology inside the resource definition is not quite what we are used to. So it was confusing even figuring out what they should be.

I've added /virtual/tenant for the two segments (makes sense)

And remove the full uri https://base.com. While it would work, I'm just not sure it makes sense.

Equally (and I think this is probably a different issue, but noone has ever raised it so I am ignoring it.)

I'm not sure how a base path would work in combination with a tenant prefix.

But I'm not sure exactly what you would use the basePath for anyway.

So I just test that it's behaviour is the same as it is currently so if it is being used by anyone we know there is no regression.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think doing file:// is a good idea. That would suggest that using the resource manager to load files was a good idea. Which it's not. Wouldn't work anyway the browser would block it - I would hope ;)

Copy link
Member

Choose a reason for hiding this comment

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

@deanmarcussen No problem, that's okay ;)

Just for info, using file:///C:/Sources/OrchardCore/src/OrchardCore.Cms.Web/App_Data/Sites/Default/Media/about-bg.jpg works on my machine

File Url

Copy link
Member Author

@deanmarcussen deanmarcussen Dec 23, 2020

Choose a reason for hiding this comment

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

Yes, but try loading a script that way, on an actual site ;)
How would I get about-bg to my computer. and in C: when I don't even have a C:

I assume you are having a laugh btw ;)

It's a hot path, so I don't want to add more linq to support something that shouldn't be supported.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't work anyway the browser would block it - I would hope ;)

Yes, it was just to show that the browser doesn't block an unc path starting with file://
It is on my local machine but i thought about something like file://someserver/somepath

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I give in ;)

Added file://

@agriffard agriffard merged commit a6af9a0 into dev Dec 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the deanmarcussen/cdnscripts branch December 23, 2020 15:55
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.

Resources CDN replaces inline script src
5 participants