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

Fix asset host support & improve output path #397

Merged
merged 8 commits into from May 18, 2017

Conversation

@javan
Copy link
Member

@javan javan commented May 17, 2017

  • Fixes duplicated asset hosts (Fixes #320, Closes #335)

  • Fixes that asset hosts would be ignored when defined as Proc or when set on ActionController::Base.asset_host directly

  • Fixes that webpacker:compile and bin/webpack would generate dev server URLs in development mode. This should only happen when using bin/webpack-dev-server.

  • Changes paths.output so that it's:

    1. Relative to public/
    2. Distinct from paths.entry
    3. Not broken if you change it. Previously webpacker would compile into paths.output/paths.entry, but wouldn't include paths.output in generated URLs. So, if you changed paths.output, you'd end up with busted URLs:

    Before:

    >> Webpacker::Configuration.paths[:entry]
    => "packs"
    
    >> Webpacker::Configuration.paths[:output]
    => "public/assets"
    
    >> Rails.application.config.action_controller.asset_host
    => "https://cdn.example.com"
    
    >> Webpacker::Manifest.lookup("application.js")
    => "https://cdn.example.com/packs/application.js"
    
    # That ^ URL would not work because the compiled file isn't there
    >> Rails.root.join("public/packs/application.js").exist?
    => false
    
    # Because the compiled file is here
    >> Rails.root.join("public/assets/packs/application.js").exist?
    => true

    After:

    >> Webpacker::Configuration.paths[:entry]
    => "packs"
    
    >> Webpacker::Configuration.paths[:output]
    => "assets"
    
    >> Webpacker::Manifest.lookup("application.js")
    => "https://cdn.example.com/assets/application.js"
    
    >> Rails.root.join("public/assets/application.js").exist?
    => true
javan added 2 commits May 17, 2017
`config.action_controller.asset_host` won’t work when:
* It’s a Proc
* It’s nil (the application may set `ActionController::Base.asset_host` directly)
@javan javan requested review from gauravtiwari and dhh May 17, 2017
Copy link
Member

@guilleiguaran guilleiguaran left a comment

Looking good

@javan javan mentioned this pull request May 18, 2017

const output = {
path: resolve('public', paths.output),
publicPath: formatPublicPath(env.ASSET_HOST, paths.output)

This comment has been minimized.

@gauravtiwari

gauravtiwari May 18, 2017
Member

How about we set ASSET_HOST to dev server host in development when it's enabled? That way we don't need to replace manifest plugin.

if (env.NODE_ENV === "development") {
  env.ASSET_HOST = `//${host}:${port}` // relative to origin
}

OR

if (env.NODE_ENV === "development") {
  env.ASSET_HOST = `${devServer.https ? 'https': 'http'}://${devServer.host}:${devServer.port}`
}

We would be able to do this in development config without if, but guess we can't since we have much shared logic here.

This comment has been minimized.

@javan

javan May 18, 2017
Author Member

I described the reason for not doing it this way in the PR:

Fixes that webpacker:compile and bin/webpack would generate dev server URLs in development mode. This should only happen when using bin/webpack-dev-server.

That said, I cleaned things up in 2a94cdf by constructing and setting ASSET_HOST in bin/webpack-dev-server.

const publicPath = formatPublicPath(`http://${host}:${port}`, paths.output)

// Remove ManifestPlugin so we can replace it with a new one
devConfig.plugins = devConfig.plugins.filter(plugin => plugin.constructor.name !== 'ManifestPlugin')

This comment has been minimized.

@gauravtiwari

gauravtiwari May 18, 2017
Member

If we consider using ASSET_HOST env in development then I guess we don't need any of the changes here :)

default: &default # ~ = Rails.root
source: app/javascript # ~/:source
entry: packs # ~/:source/:entry
output: packs # ~/public/:output

This comment has been minimized.

@gauravtiwari

gauravtiwari May 18, 2017
Member

Awesome 👍 been thinking to do this

instance.data
end

def default_paths

This comment has been minimized.

@gauravtiwari

gauravtiwari May 18, 2017
Member

🍰 🎉

Copy link
Member

@dhh dhh left a comment

Looks good to me!

@javan javan merged commit ebcc609 into rails:master May 18, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@javan javan deleted the javan:customize-output-path branch May 18, 2017
@mltsy
Copy link

@mltsy mltsy commented Jul 27, 2017

You guys are awesome! Thanks for working on this feature 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants