Fix asset host duplication #335
Conversation
| const { slashes, href } = new Url(host) | ||
| if (slashes) { return `${href}/${paths.entry}/` } | ||
| if (href) { return `//${href}/${paths.entry}/` } | ||
| return `/${paths.entry}/` |
There was a problem hiding this comment.
This is a bit confusing flow. Can we replace with a case statement instead? Not a big fan of multiple exit functions like this.
There was a problem hiding this comment.
Yepp, me don't like it either but seems we can't fully use switch as we are dependent on multiple params. I will break into two functions.
|
|
||
| module.exports = { | ||
| test: /\.(jpg|jpeg|png|gif|svg|eot|ttf|woff|woff2)$/i, | ||
| use: [{ | ||
| loader: 'file-loader', | ||
| options: { | ||
| publicPath, | ||
| publicPath: env.NODE_ENV === 'production' ? computeAssetPath(env.ASSET_HOST) : publicPath, | ||
| name: env.NODE_ENV === 'production' ? '[name]-[hash].[ext]' : '[name].[ext]' |
There was a problem hiding this comment.
Since both of these options differ on the same conditional, we should extract the conditional to a higher level. Maybe:
cont assetOptions = (env) => {
if (env.NODE_ENV === 'production') {
return { publicPath: computeAssetPath(env.ASSET_HOST), name: '[name]-[hash].[ext]' }
} else {
return { publicPath: publicPath, name: '[name].[ext]' }
}
}
options: assetOptions(env)
|
@dhh If you are happy we can merge this one 😄 |
| // Make sure host has slashes | ||
| const ensureSlashes = ({ href, slashes }) => { | ||
| if (slashes) { return `${href}/${paths.entry}/` } | ||
| return `//${href}/${paths.entry}/` |
There was a problem hiding this comment.
This should be a regular if/else conditional, rather than a double exit function. Same with the other methods here.
There was a problem hiding this comment.
Yepp, done the same when you suggested the other day but it seems eslint doesn't like it. If an if block contains a return statement, the else block becomes unnecessary. (one of the JS things 😉 )
|
Screw eslint then. Either there must be a setting for that or we will just ignore what it has to say on this topic.
… On May 10, 2017, at 18:59, Gaurav Tiwari ***@***.***> wrote:
@gauravtiwari commented on this pull request.
In lib/install/config/loaders/core/assets.js:
> @@ -1,12 +1,30 @@
-const { env, publicPath } = require('../configuration.js')
+const Url = require('url-parse')
+const { env, paths, publicPath } = require('../configuration.js')
+
+// Make sure host has slashes
+const ensureSlashes = ({ href, slashes }) => {
+ if (slashes) { return `${href}/${paths.entry}/` }
+ return `//${href}/${paths.entry}/`
Yepp, done the same when you suggested the other day but it seems eslint doesn't like it. If an if block contains a return statement, the else block becomes unnecessary. (one of those JS things 😉 )
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Screwed 😄 - 8fcaf18 |
|
Can you show what a compiled |
|
@javan I am now confused too 😄 Hang on, checking... |
|
Just checked, so this change is for internal assets linked within components that don't directly get referenced from manifest and it will be embedded inside source after compile -
Previously, I have done this for all, forgetting Rails helper tag already does this too so the CDN url was included twice per #320 |
|
Slow poke @javan 😄 |
|
Rails already knows how to do this in Ruby. See compute_asset_host. How about we just use that instead of duplicating the same logic in JavaScript? --- a/lib/tasks/webpacker/compile.rake
+++ b/lib/tasks/webpacker/compile.rake
@@ -6,7 +6,7 @@ namespace :webpacker do
desc "Compile javascript packs using webpack for production with digests"
task compile: ["webpacker:verify_install", :environment] do
puts "Compiling webpacker assets <U+1F389>"
- asset_host = Rails.application.config.action_controller.asset_host
+ asset_host = ApplicationController.helpers.compute_asset_host |
Fix asset host duplication Revert change in shared.js Return if no host is provided Remove https Remove asset host Change function name
There was a problem hiding this comment.
+1 for @javan suggestion, let's try to use compute_asset_host helper for this
it's possible to use the helper without depending on an existing ApplicationController ??
|
Can use |
|
@javan cool!! 👍 |
|
@javan @guilleiguaran Yepp thought about using that but don't want to couple too much with Rails internals so the Gem works with other older Rails versions and standalone ruby apps (may be something that we can move towards?). There is some duplication but I guess it makes sense if we are thinking to support larger community. What do you think? |
|
Anyone have an update on where this is at? This causing my JS assets to not work on staging/production. Is there a temp workaround? |
|
@catskull Don't use CDN for now or update configuration.js to this please |
|
or in your Gemfile gem 'webpacker', github: 'gauravtiwari/webpacker', branch: 'fix-asset-host' |
|
Thanks, using your configuration.js works! |
|
Fixed + more in #397 |
Fixes #320 and other linked issues around asset host.
Since we are using Rails asset helper tag internally, CDN is applied automatically on all top level webpack assets, however the internal JS assets like images and fonts don't use CDN.
Previously in #224 a fix was introduced but it broke the expected behaviour - basically we were applying ASSET_HOST twice. This PR fixes this and only applies ASSET_HOST to internal file loader assets.