Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Correctly generate cdn base url for local scripts #8042
Changes from 4 commits
8687156
426a0e5
3987ed5
c0d06ea
24c9673
24f982d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When mapping to a virtual path
Then in our middleware we set
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
That said in the tests, for me
/tenant
and/virtual/tenant
are notapplicationPath
, butbasePath
, and then in/tenant/scripts/script.js
,/scripts/script.js
is a pathOkay just saw in the
ResourceDefinition
code the usage ofGetTagBuilder(..., string applicationPath, ...)
which is called somewhere byGetHtmlContent(string appPath)
which is called byGetHtmlContent(_options.ContentBasePath)
, and where_options.ContentBasePath
is initialized with the httpRequestPathBase
;)So okay, we could say that
applicationPath
is the same asbasePath
, but here i preferbasePath
, at leasttenantPath
as an application can have multiple tenants, but each one being a kind of an isolated app ;)https://base.com
is not abasePath
, nor anapplicationPath
, but a full Uri (Url is okay), and e.g. in the full urihttps://base.com/virtual/tenant/scripts/script.js
,/virtual/tenant
is a basePath and/scripts/script.js
is a pathMaybe good to have a test with
/virtual/tenant
, just to do a test with a basePath having 2 segmentsBut as said it's just a question of terminology, so as you want ;)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ;)There was a problem hiding this comment.
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 machineThere was a problem hiding this comment.
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 inC:
when I don't even have aC:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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://