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 distDir layout for advanced webpack features #127

Closed
wants to merge 1 commit into from

Conversation

dleavitt
Copy link
Contributor

@dleavitt dleavitt commented Feb 27, 2017

Right now this gem is built on the assumption that app/javascript/packs will always match public/packs. This holds when the JS entry points are the only files emitted and avoids users having to mess with publicPath in the webpack config.

But this isn't going to be the case for most webpack-based apps of any complexity. Eg, file-loader breaks this assumption. I believe it gets broken under a number of other scenarios as well: commons chunks, ExtractTextPlugin, some kinds of code splitting, etc.

These are not scenarios we need to support out-of-the-box, but per #109 it makes sense to set up defaults that make them possible.

The easiest solution I could think of was to make the structure of the "packs" dir match the structure of app/javascript.

Let's say your app/javascript dir looked like this:

app/javascript
+-- images
|   +-- image.png
+-- packs
|   +-- application.js
|   +-- admin.js
+-- nonentry.js

If you ran bin/webpack, your public/packs dir would look like this:

public/packs
+-- images
|   +-- image.png
+-- application.js
+-- admin.js

Doh! Images is now a sibling of your pack files! The structure doesn't match the structure of app/javascript, causing the need for additional configuration and pain. Instead let's make it look like this:

public/dist
+-- images
|   +-- image.png
+-- packs
|   +-- application.js
|   +-- admin.js

Nice, it matches! I also renamed the top-level dir from packs to dist to make clear that it's not the equivalent of the packs dir in app/javascript, and because assets was already taken.

Thoughts? Any lower-impact alternatives?

cc @gauravtiwari

@guilleiguaran
Copy link
Member

/cc @dhh

@gauravtiwari
Copy link
Member

gauravtiwari commented Feb 28, 2017

@dleavitt Good idea, however shall we wait until we have the other changes we discussed in #109 in place with a PR so, we can bring it all together and discuss it through. Probably, will have more context on this then :)

@dleavitt
Copy link
Contributor Author

Yeah, I left this open because I was hoping to get some feedback on this from somebody with knowledge of webpack internals. Haven't gotten around to poking people yet though.

@dleavitt dleavitt closed this Feb 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants