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
[Threescale 7522] Fix CDN urls for fonts assets #3072
[Threescale 7522] Fix CDN urls for fonts assets #3072
Changes from 3 commits
2c3e4a8
e0010c3
295a331
6e9c99d
f29bb2d
468c0db
44c1e54
19f3fc0
401fb66
3f9cdf0
9d05346
727a33c
f67119c
13398a2
a9a771f
90fb76b
8a79e58
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.
Not sure I get it - there are two
asset_host
values in different places in config tree? 🤔UPDATE: Didn't scroll enough 😅
If I understand correctly
Rails.configuration.asset_host
is coming fromconfig/environments/{env}.rb
, andRails.configuration.three_scale.asset_host
comes fromsettings.yml
.From the change and the comment in webpack config, it seems to me that we always assume that this asset_host should be empty on compilation time?
If so, shouldn't we force it to be empty, so we don't have double-host issues?
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.
Yes.
Do you mean force it empty during compilation time? It's the same @akostadinov said. Yes, I agree.
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 added a callback that returns an error during assets compilation if the asset host is set. I couldn't find an easy way to just unset the asset host transparently. That would need more time but I can try if you guys think it's needed.
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.
It is still not clear for me why we are using both settings...
Rails.configuration.asset_host
andRails.configuration.three_scale.asset_host
Shouldn't we use just one?...
I guess
Rails.configuration.asset_host
is the one that should define the actual value, as it's env-specific, and will use the value inRails.configuration.three_scale.asset_host
, if present.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.
The relevant piece of code is here: /config/environments/production.rb#L109-L121
We could just set
Rails.configuration.asset_host
toRails.configuration.three_scale.asset_host
and it would work. The first was converted into a lambda function here. I can't get too much info from the commit message: what other assets besides ours are we loading throughjavascript_include_tag
? Maybe we can remove the lambda function.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.
Hm, I am not sure about the reason for having this function there.
What I mean is: do you think we can we just rely on
Rails.configuration.asset_host
in this helper?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.
We need both variables,
asset_host_enabled
tells us when the CDN is enabled, it can be disabled whenDISABLE_DIGEST == 1
. In that case we don't care about the value ofRAILS_ASSETS
: all assets loaded by rails won't include the CDN url, so the ones loaded by webpack shouldn't neither.It's also possible that
asset_host_enabled
is true, but there's not provided asset url (asset_host_url = RAILS_ASSET_HOST = <empty>
), then the situation is the same, rails won't load any asset and webpack shouldn't neither. So both variables must be true to go ahead and return the CDN url for webpack.If we rely only on the value of
asset_host_enabled
, then we need to check for the value ofasset_host_url
anyway, to decide if we prepend thehttps://
prefix or not. Now it's done in a way all possibilities are checked in one line: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.
In this case, does it make sense to rely only on
asset_host_url
, if it is set, then prepend. WDYT? Or isasset_host_enabled
a rails thing that is tricky to ignore?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.
If we rely only in
asset_host_url
, the second row in the table would fail, it would return'https://whtaever.cloudfront.net/'
instead of''
. Sinceasset_host_enabled == false
, it will prepend the URL to webpack assets like fonts and JS files, but it won't use the CDN for all other assets. I think that's not the expected behaviour, if the admin disables the CDN, then it must be disabled for all assets.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.
If admin wants to disable CDN, admin could empty
asset_host_url
. Or there is some other issue related to that?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.
It can be disabled in two ways: by
DISABLE_DIGEST == 1
or byRAILS_ASSET_HOST == ''
. If the admin disables the CDN by leavingRAILS_ASSET_HOST
empty, that will work, but if they want to enable it solely byRAILS_ASSET_HOST
it might not work becauseDISABLE_DIGEST
could be 1, and then it would only enable the CDN for webpack but not for Sprockets. We need to enforce that no matter how the assets are disabled or enabled, they must be disabled or enabled for both webpack and sprockets.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 think there's no need to check the string twice. Would this be equivalent?
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.
It's not checking the string twice because only
asset_host_url
is a string,asset_host_enabled
is a boolean. This comment explains why both are required.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 think it's not the only layout that is used in the app... probably need to add this in others as well.
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 think I updated it in all layouts
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.
Is this due to the upgrade?
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: https://stackoverflow.com/questions/64363427/webpacker-the-resolved-paths-option-has-been-deprecated-use-additional-paths-i
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.
That is a bold move 😄 No breaking changes or caveats?
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.
Don't you trust me? 😢
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 didn't try this thoroughly, I only used it in my development environment and my dev cluster and noticed no incidences, also all tests pass. So how could I be sure I'm not breaking anything?
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.
You can probably read through the changelog for any breaking changes I guess.
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.
This is the list of breaking changes:
Only the nodejs version seems relevant for me. Which one is installed in SaaS?