Adds asp-append-version support #3581

deanmarcussen · 2019-05-05T14:05:23Z

This adds asp-append-version support to media, style, and script tag helpers, and liquid filters, through providing a replacement for the MVC DefaultFileVersionProvider, and adaptions to the various filters, tag helpers, and ResourceManager

It replaces #3172 as @jtkech has provided lots of advise as we've come up with a better way to achieve this.

It covers issues which are collected together in #3537 regarding asp-file-version support

Media is covered through changes to the ImageTagHelper

Scripts and styles, are covered through changes to the Tag Helpers and a new property on the ResourceSettings of UseAppendVersion so it can also be configured through code.

I've updated the admin theme and ResourceManifest to UseAppendVersion - it is helpful also for registered resources, as will provide a version tag when in debug mode and serving, for example jQuery which in debug mode does not have a version string in our local path to it. More updates to do to other Admin related views in other modules, if we want to use this as a default feature.

It also adds a cdn base path property to ResourceManager and site settings to allow only locally assets to be prefixed with a default cdn url. (to see in action just use your https://localhost address as the base)

And while I was there I added the code from O1 OrchardCMS/Orchard#8176 to support better resource bundling from issue #3434

@jtkech I've left #3172 open for the moment as we have so many discussions about further supporting blob storage. Cheers for all your help! (and probably more help with reviewing!)

…n. AppendVersion remains useful on ResourceManifest that also uses Version as when providing debug-src url's are not versioned

… so UseStaticFiles can resolve correct IFileProvider, in case another IFileProvider registered earlier (no conflict with media or custom registration in Cms.Web for example)

…added OrchardCMS#8177 / OrchardCMS#8177 from Orchard V1

… within markdown & html body

…add the OC FileVersionProvider

… file provider manifest to once per tenant = false

src/OrchardCore.Modules/OrchardCore.Media/Filters/MediaFilters.cs

jtkech · 2019-05-06T07:43:09Z

@deanmarcussen

Just tried, i could see the style / script appended versions when under the admin, looks very good. I may not have so much time this week (administrative things in late) but i will review it asap, maybe step by step.

Right now i just reviewed the AppendVersionFilter liquid filter, nothing very important, mainly to describe my findings when testing it.

…o provide additional path base information to IFileProviders if required.

deanmarcussen · 2019-05-06T09:50:05Z

@jtkech Thanks for the quick look and comments!

I've since implemented the IVirtualPathBaseProvider discussed above and is working better in (I hope) all scenarios

OC Tag Helpers
Mvc Tag Helpers
Liquid filters and Liquid -> mvc bridge
Multi tenanted and single host

I also implemented the same IFileProvider pattern for the OrchardCore.Tenants.FileProvider feature as I had totally forgotten about it! Have also included the old request from you for the Manifest to be changed to DefaultTenantOnly = false

And done some code cleanup (my bad)

Cheers appreciate the time it takes to review! Busy week for me on other things as well.

jtkech · 2019-05-06T22:23:13Z

@deanmarcussen

I like it 👍 just did some refactoring on my side that i will explain through comments or a review coming soon. Please don't push other updates because i did the changes on my side for testing, so would be better that i could push them to your branch, or create a branch on my side that you will merge.

src/OrchardCore.Modules/OrchardCore.Media/Filters/MediaFilters.cs

src/OrchardCore.Modules/OrchardCore.Media/Startup.cs

src/OrchardCore.Modules/OrchardCore.Tenants/Manifest.cs

src/OrchardCore.Modules/OrchardCore.Tenants/Services/ITenantFileProvider.cs

src/OrchardCore/OrchardCore.Abstractions/Modules/IModuleStaticFileProvider.cs

src/OrchardCore/OrchardCore.Abstractions/Modules/ModuleCompositeFileProvider.cs

src/OrchardCore/OrchardCore.ResourceManagement/FileVersionProvider.cs

src/OrchardCore/OrchardCore.ResourceManagement/ServiceCollectionExtensions.cs

src/OrchardCore/OrchardCore/Modules/Extensions/ServiceCollectionExtensions.cs

jtkech · 2019-05-07T03:47:21Z

@deanmarcussen

So, i did a review, mainly some renaming, and moving some implementations e.g to make ResourceManagement still independent of OC.
I tried the cdn url setting to use a cdn for our local static files and that still allow to append a version hash. I tried it by using http://localhost:xxxx, it works fine.

As said above i did the changes on my side for testing, so we will see tomorrow how we can share my work so that you can review it.

deanmarcussen · 2019-05-07T06:02:20Z

@jtkech Great, review looks very good - I'm still figuring out where to put things in the many Orchard Projects.

Really good to move FileVersionProvider out of ResourceManagement - I wanted to but had no idea where to put it.

Also I had named it FileVersionProvider rather than DefaultFileVersionProvider as that was MVC's name for it, but it might be better as DefaultFileVersionProvider?

I think maybe your review allows me to clean up a few other things so maybe when you are finished reviewing, if you pull request my branch, then I can tidy up some more things that occur to me now.

Regardless I make no more commits to this PR while you are still looking through it.

Also still not sure if I need to add so many File Watchers for Not Found files, but seemed useful (especially for testing by dropping missing files into folders), but if you have any thoughts on this be great.

jtkech · 2019-05-07T20:31:42Z

@deanmarcussen

I did some answers on the review.

Also I had named it FileVersionProvider rather than DefaultFileVersionProvider as that was MVC's name for it, but it might be better as DefaultFileVersionProvider?

I renamed it to ShellFileVersionProvider.

I think maybe your review allows me to clean up a few other things so maybe when you are finished reviewing, if you pull request my branch, then I can tidy up some more things that occur to me now.

Okay.

Also still not sure if I need to add so many File Watchers for Not Found files, but seemed useful (especially for testing by dropping missing files into folders), but if you have any thoughts on this be great.

Yes, useful in dev mode (or not) for files that are still physical files, so that the token is invalidated when the file is removed / added. Hmm, maybe not so useful in prod because most of the files are embedded, but there are app level static files, tenant specific ones and medias, so right now let keep it, will see.

Update: Do we have to update all razor / liquid views to use the append version attribute? Or later in another PR ;)

Update: I pushed my updates on your branch, please review them, i may have missed something, did wrong things, some comments not correctly updated, and maybe re-do some tests.

jtkech · 2019-05-09T22:29:54Z

Note I realise I forget to do the LinkTagHelper so I do that tonight

I forgot it me too ;)

I let you test it, let me know when you are ready with this last change so that i can approve this PR, then for merging it we will have to wait for @sebastienros ;)

Update: Next PR

Make media azure tenant aware, blob images resizable and versionable, and compatible with our tag helpers. Maybe not so simple but i think it will be easier with this new base.

deanmarcussen · 2019-05-10T06:33:54Z

@jtkech Tested again this morning, and all working well, so for me it is ready. Cheers for your herlp getting it knocked into shape!

@sebastienros FYI, below is screenshot for Resource Cdn settings - this is the only visual change in the admin.

Update: Next PR
Make media azure tenant aware, blob images resizable and versionable, and compatible with our tag helpers. Maybe not so simple but i think it will be easier with this new base.

Yes, @sebastienros as discussed, input when you are ready on design :)

Other Next PR
While we work on design for Azure, will update admin views / media picker / rich text editor filter / etc al to use append version

Skrypt · 2019-05-29T23:35:08Z

I agree that source maps should be generated separately from minified assets. The source maps should be only usefull for dev purpose and never be published on prod websites. The gulp pipeline as it is has been migrated from 01 and has only been slightly modified for OC. I'm opening an issue for this and other issues reported with the gulp pipeline.

Skrypt · 2019-05-29T23:44:44Z

So basically this feature is : cache busting for assets.

jtkech · 2019-05-30T00:05:19Z

Yes, and the new conflict is because of my last PR ;)

Skrypt · 2019-05-30T00:22:25Z

Ohh you bad french people! 🍪 🍪

Skrypt · 2019-05-30T09:13:42Z

Ok I tested this PR and with Liquid so far everything works fine.

deanmarcussen · 2019-05-30T10:09:25Z

@Skrypt Yes cache busting for js/css and media assets

Cheers for testing 😄

I'll resolve merge conflict shortly

deanmarcussen · 2019-05-30T10:29:54Z

Oh also, you should be able to see it working with razor on the admin page layout
view-source:https://localhost:44300/admin

e.g.

<link href="/TheAdmin/Styles/TheAdmin.css?v=zJ9dsRDaOTdF7jaJ5f3nHnoHp2HRzSkgSeXTZmg4sy0" rel="stylesheet" type="text/css" />

<!-- This script can't wait to the footer -->
<script src="/TheAdmin/Scripts/userPreferencesLoader.js?v=eER9OO6zWGKaIq1RlNjImsrWN9y2oTgQKg2TrJnDUWk" type="text/javascript"></script>

Note Only the admin Layout.cshtml has been altered to use asp-append-version

Skrypt · 2019-05-30T12:03:26Z

I was looking at the assets for our Admin theme and there's probably things that could be bundled together. The frontend themes have the same issue but we kept the templates as close as they we're built without really optimizing them properly by using only one .js file and one .css file per template. We have a gulp pipeline but we are not using it optimally. As for CDN for assets, the more you add external libraries in your HTML the more requests you get on your page and the slower it gets. So far it has always been faster for me to bundle everything in one file if I could.

I'm going to work on something today. At least fix the issues that we all have with Bootstrap versions.

deanmarcussen · 2019-05-30T12:36:58Z

Couldn't agree more.
Maybe one site / one vendor file with good cache headers, and cache busting, is my preferred solution, though I have been experimenting recently with using bootstrap and common vendor files from their cdn's in the hope that the end user already has it cached. Ruins the generic seo speed tests, as they never cache etc.

Good luck in the seventh circle of npm hell!

Skrypt · 2019-06-23T17:19:39Z

I think @sebastienros wants to look at the files watcher code before merging. It's working nicely on my website on prod as of now but I'm not using Azure Blob Storage yet.

deanmarcussen · 2019-06-23T18:15:04Z

ah good to know it's working!

The file watcher code is straight out of the asp-net default handler.

I think it watches every 4 seconds (might be 2).

I do question how useful it is to have a file watcher running on the media dir. Could just as easily do the cache clearing from the MediaFileStore on file upload. Might work better with Blob Storage if it did that as well

sebastienros · 2019-07-04T19:40:52Z

Please merge when the conflicts are solved. We'll still need to add the CDN settings for the Media items.

jtkech · 2019-07-04T21:02:03Z

Okay, i will do

# Conflicts: # src/OrchardCore.Modules/OrchardCore.Media/Startup.cs # src/OrchardCore/OrchardCore.Mvc.Core/Startup.cs

* MediaFileStoreVersionProvider and FileVersionHashProvider * WIP. Add IFileVersion to ResourceManager and Tag Helpers * WIP Update ResourceManifest and admin layout to use asp-append-version. AppendVersion remains useful on ResourceManifest that also uses Version as when providing debug-src url's are not versioned * create IModuleStaticFileProvider and provide registration identifiers so UseStaticFiles can resolve correct IFileProvider, in case another IFileProvider registered earlier (no conflict with media or custom registration in Cms.Web for example) * WIP. Media ImageHelper working but needs further testing * add ImageVersionProcessor suggest by jtkech. Tested works well * Fixes OrchardCMS#3434 Setting the right resource key in debug mode - added OrchardCMS#8177 / OrchardCMS#8177 from Orchard V1 * add liquid tag helper for append_version * updated LiquidFilter to work via httpContextAccessor to support media within markdown & html body * implement cache / tidy up mediafilter * remove IFileVersionHashProvider, and Replace, rather than remove and add the OC FileVersionProvider * renamed MediaFileImageProvider to MediaResizingFileProvider * add CdnBaseUrl support for prepending a base cdn url to assets * update documentation, and support append-version with AssertUrl Helper * documentation tweaks * more documentation tweaks * add support for tenant static file provider, and change tenant static file provider manifest to once per tenant = false * Add support for IVirtualPathBaseProvider as a better generic filter to provide additional path base information to IFileProviders if required. * Code cleanup * Refactoring * Minor changes. * Refactoring ShellFileVersionProvider * Refactoring, rename ImageTagHelper Attribute asp-append-version * Also use a shared cache for module static files. * Minor change. * Add support for AppendVersion to Link Tag Helper * Little formatting after merging dev.

deanmarcussen added 18 commits April 30, 2019 20:40

MediaFileStoreVersionProvider and FileVersionHashProvider

d6af96f

WIP. Add IFileVersion to ResourceManager and Tag Helpers

ca10230

Merge branch 'dev' into file-version-providers

b1c17bc

WIP Update ResourceManifest and admin layout to use asp-append-versio…

0541823

…n. AppendVersion remains useful on ResourceManifest that also uses Version as when providing debug-src url's are not versioned

create IModuleStaticFileProvider and provide registration identifiers…

b23d4a6

… so UseStaticFiles can resolve correct IFileProvider, in case another IFileProvider registered earlier (no conflict with media or custom registration in Cms.Web for example)

WIP. Media ImageHelper working but needs further testing

6eac4c1

add ImageVersionProcessor suggest by jtkech. Tested works well

303c3b3

Fixes OrchardCMS#3434 Setting the right resource key in debug mode - …

5b090ac

…added OrchardCMS#8177 / OrchardCMS#8177 from Orchard V1

add liquid tag helper for append_version

a78bd7c

updated LiquidFilter to work via httpContextAccessor to support media…

55eb778

… within markdown & html body

implement cache / tidy up mediafilter

fd70d8f

remove IFileVersionHashProvider, and Replace, rather than remove and …

a5e8b65

…add the OC FileVersionProvider

renamed MediaFileImageProvider to MediaResizingFileProvider

6890dc3

add CdnBaseUrl support for prepending a base cdn url to assets

7e886d2

update documentation, and support append-version with AssertUrl Helper

aa05c9d

documentation tweaks

6e103c7

more documentation tweaks

bb8b9d2

add support for tenant static file provider, and change tenant static…

b48595d

… file provider manifest to once per tenant = false

jtkech reviewed May 6, 2019

View reviewed changes

deanmarcussen added 2 commits May 6, 2019 10:14

Add support for IVirtualPathBaseProvider as a better generic filter t…

413b0b4

…o provide additional path base information to IFileProviders if required.

Code cleanup

b71cff3

deanmarcussen mentioned this pull request May 6, 2019

Media CdnBaseUrl Configuration #3468

Closed

jtkech reviewed May 7, 2019

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Media/Filters/MediaFilters.cs Outdated Show resolved Hide resolved

jtkech reviewed May 7, 2019

View reviewed changes

jtkech approved these changes May 11, 2019

View reviewed changes

Merge branch 'dev' into file-version-providers

40e32f8

merge dev, resolve conflicts

83ae789

sebastienros approved these changes Jul 4, 2019

View reviewed changes

jtkech added 2 commits July 5, 2019 01:13

Merge remote-tracking branch 'oc-origin/dev' into file-version-providers

3382ab5

# Conflicts: # src/OrchardCore.Modules/OrchardCore.Media/Startup.cs # src/OrchardCore/OrchardCore.Mvc.Core/Startup.cs

Little formatting after merging dev.

4563054

jtkech merged commit b3508d5 into OrchardCMS:dev Jul 5, 2019

dodyg mentioned this pull request Jul 7, 2019

Implementation notes for decoupled CMS #3553

Closed

deanmarcussen deleted the file-version-providers branch July 7, 2019 14:35

deanmarcussen mentioned this pull request Jul 13, 2019

asp-append-version="true" does not work in theme files #3169

Closed

JoeBerkley mentioned this pull request Jul 29, 2019

Add cache busting for CSS & JavaScript assets EtchUK/Etch.OrchardCore.ThemeBoilerplate#7

Closed

deanmarcussen mentioned this pull request Oct 24, 2019

Static File Provider not working on tenants besides default #4626

Closed

deanmarcussen mentioned this pull request Jun 26, 2020

Integrate MVC "Make FileVersionProvider Replaceable" Improvements When Available #2435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds asp-append-version support #3581

Adds asp-append-version support #3581

deanmarcussen commented May 5, 2019

jtkech commented May 6, 2019

deanmarcussen commented May 6, 2019

jtkech commented May 6, 2019

jtkech commented May 7, 2019

deanmarcussen commented May 7, 2019

jtkech commented May 7, 2019 •

edited

jtkech commented May 9, 2019 •

edited

deanmarcussen commented May 10, 2019 •

edited

Skrypt commented May 29, 2019

Skrypt commented May 29, 2019

jtkech commented May 30, 2019

Skrypt commented May 30, 2019

Skrypt commented May 30, 2019

deanmarcussen commented May 30, 2019

deanmarcussen commented May 30, 2019

Skrypt commented May 30, 2019 •

edited

deanmarcussen commented May 30, 2019

Skrypt commented Jun 23, 2019 •

edited

deanmarcussen commented Jun 23, 2019

sebastienros commented Jul 4, 2019

jtkech commented Jul 4, 2019

Adds asp-append-version support #3581

Adds asp-append-version support #3581

Conversation

deanmarcussen commented May 5, 2019

jtkech commented May 6, 2019

deanmarcussen commented May 6, 2019

jtkech commented May 6, 2019

jtkech commented May 7, 2019

deanmarcussen commented May 7, 2019

jtkech commented May 7, 2019 • edited

jtkech commented May 9, 2019 • edited

deanmarcussen commented May 10, 2019 • edited

Skrypt commented May 29, 2019

Skrypt commented May 29, 2019

jtkech commented May 30, 2019

Skrypt commented May 30, 2019

Skrypt commented May 30, 2019

deanmarcussen commented May 30, 2019

deanmarcussen commented May 30, 2019

Skrypt commented May 30, 2019 • edited

deanmarcussen commented May 30, 2019

Skrypt commented Jun 23, 2019 • edited

deanmarcussen commented Jun 23, 2019

sebastienros commented Jul 4, 2019

jtkech commented Jul 4, 2019

jtkech commented May 7, 2019 •

edited

jtkech commented May 9, 2019 •

edited

deanmarcussen commented May 10, 2019 •

edited

Skrypt commented May 30, 2019 •

edited

Skrypt commented Jun 23, 2019 •

edited