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

Update Smidge Config to reflect the UmbracoSmidgeConfigCacheBuster #3891

Merged
merged 8 commits into from Jul 7, 2022

Conversation

marcemarc
Copy link
Contributor

No description provided.

@marcemarc marcemarc requested a review from p-m-j April 1, 2022 16:07
@marcemarc
Copy link
Contributor Author

hi @p-m-j I've just been puzzling over why the docs setting of smidge version doesn't result in a v12345 on the assets, and saw your update to the UmbracoSmidgeConfigCacheBuster - I've updated the explanation here to hopefully reflect your changes, be ace if you had a moment to cast your eye over it!

@p-m-j
Copy link
Contributor

p-m-j commented Apr 2, 2022

Hey @marcemarc, I added some comments with my $0.02, the docs team are much better at writing style than I am so probably better to wait for their input as opposed to mine.

Looks good though thanks for the PR

@p-m-j
Copy link
Contributor

p-m-j commented Apr 2, 2022

Might be better to avoid explicit mention of smidge as its use is just an implementation detail of the umbraco RuntimeMinification abstraction.

marcemarc and others added 3 commits April 2, 2022 21:50
Co-authored-by: Paul Johnson <pmj@umbraco.com>
Co-authored-by: Paul Johnson <pmj@umbraco.com>
Co-authored-by: Paul Johnson <pmj@umbraco.com>
@marcemarc
Copy link
Contributor Author

Hey @marcemarc, I added some comments with my $0.02, the docs team are much better at writing style than I am so probably better to wait for their input as opposed to mine.

Looks good though thanks for the PR

Thanks @p-m-j! - will revert to the docs team! they do an awesome job...

"CacheBuster": "Version",
"Version": "1234"
}
}
}
```
The actual 'Version' number will not be visible in the url of the assets, this is because it is combined, along with the Umbraco Version from configuration and the your project assembly dll, and then once combined a 'hash' is generated to obscure these details.

in the HTML link thus: ```<link href='/sc/69a3dbf6.1cf661e7.css.d=efb7329470a16d7020272a294742726ebe1da5f1' rel='stylesheet' type='text/css'/>```
Copy link
Contributor

Choose a reason for hiding this comment

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

Using Version cache buster the output in v9.4.1 is like this

<link href='/sb/umbraco-backoffice-init-css.css.v7a71f91360259c5f7c3337f152b0df01eeee36f0' rel='stylesheet' type='text/css'/>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@p-m-j p-m-j Apr 4, 2022

Choose a reason for hiding this comment

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

This changes based on the value of Hosting:Debug.

I'm not sure the emphasis on the hash appearing is useful as long as users are aware of when then cache busting takes place.

However I do wonder if we should change the value to be {{hash}}.{{configured explicit version value}} so the value from config shows in the URL to avoid confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

However I do wonder if we should change the value to be {{hash}}.{{configured explicit version value}} so the value from config shows in the URL to avoid confusion.

I think this makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However I do wonder if we should change the value to be {{hash}}.{{configured explicit version value}} so the value from config shows in the URL to avoid confusion.

Yes, great observation! - it's the 'setting' of an explicit version in the appsettings configuration that then doesn't translate to what's seen on the asset URLs, that causes confusion!

@Jeavon
Copy link
Contributor

Jeavon commented Apr 4, 2022

@p-m-j whilst figuring this out with @marcemarc it has also occurred to me that I think Umbraco should be setting the Smidge dataFolder to the Umbraco Temp or at least providing the option to set it, what do you think?

@p-m-j
Copy link
Contributor

p-m-j commented Apr 4, 2022

@Jeavon I think that's already covered although perhaps it could be clearer, see final paragraph on RuntimeMinificationSettings

@Jeavon
Copy link
Contributor

Jeavon commented Apr 4, 2022

@p-m-j but setting dataFolder to a path isn't going to observe the LocalTempStorageLocation setting which I think you should be able to do. Perhaps can be done in code...

@p-m-j
Copy link
Contributor

p-m-j commented Apr 4, 2022

Ahh I see what you mean, sounds like a good idea, also useful for working towards supporting WEBSITE_RUN_FROM_PACKAGE

@Jeavon
Copy link
Contributor

Jeavon commented Apr 4, 2022

Ahh I see what you mean, sounds like a good idea, also useful for working towards supporting WEBSITE_RUN_FROM_PACKAGE

Very true! I had a quick try at setting it in code but couldn't get it swapped in time so I think should be added into Umbraco

Sorry for the whitespace I just want to check that Vale Linter is still running
@sofietoft
Copy link
Contributor

sofietoft commented Jul 7, 2022

Hi @Jeavon and @marcemarc ..

What's the status on this PR - except for the fact that there are merge conflicts?
Ready to merge, or are there still some pending questions/comments, etc?

@Jeavon
Copy link
Contributor

Jeavon commented Jul 7, 2022

@p-m-j do you have a note about setting Smidge to observe LocalTempStorageLocation or should I raise a CMS issue?

@Jeavon Jeavon merged commit 5c1951f into main Jul 7, 2022
@Jeavon Jeavon deleted the UmbracoSmidgeConfigCacheBuster branch July 7, 2022 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants