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

Correct file loader path for media outside of context #1973

Merged
merged 2 commits into from
Mar 3, 2019

Conversation

gauravtiwari
Copy link
Member

@gauravtiwari gauravtiwari commented Mar 3, 2019

Updated file loader to have filename based on the path. This change
keeps the old behaviour intact i.e. let people use namespaces for media
inside app/javascript and also include media outside of app/javascript
with simpler paths, for example from node_modules or app/assets

# Files inside app/javascript (i.e. packs source path)
# media/[full_path_relative_to_app_javascript]/name_of_the_asset_with_digest

media/images/google-97e897b3851e415bec4fd30c265eb3ce.jpg
media/images/rails-45b116b1f66cc5e6f9724e8f9a2db73d.png
media/images/some_namespace/google-97e897b3851e415bec4fd30c265eb3ce.jpg

# Files outside app/javascript (i.e. packs source path)
# media/[containing_folder_name]/name_of_the_asset_with_digest

media/some_assets/rails_assets-f0f7bbb5.png
media/webfonts/fa-brands-400-4b115e11.woff2

This change is done so we don't end up paths like media/_/assets/images/rails_assets-f0f7bbb5ef00110a0dcef7c2cb7d34a6.png or media/_/_/node_modules/foo-f0f7bbb5ef00110a0dcef7c2cb7d34a6.png for media outside of
app/javascript

Properly fixes: #1938, #1915, #1947, and #1922

@gauravtiwari
Copy link
Member Author

Does this change work for everybody? @gaffneyc @JanStevens @dwightwatson

Apologies for the trouble :) The main reason for introducing flat structure was that it's easy to work with - including in the views and has cleaner paths (outside of pack source) but I see since we have been using this approach for a while it's quite a big change, hence this PR.

@gauravtiwari
Copy link
Member Author

Perhaps, we can move to use just the [folder] name instead of [path] but again it would be a big change for people who are heavily using a nested structure and including them in views.

@gauravtiwari gauravtiwari mentioned this pull request Mar 3, 2019
@gauravtiwari gauravtiwari merged commit 938f060 into master Mar 3, 2019
@gauravtiwari gauravtiwari deleted the proper-file-loader branch March 3, 2019 21:15
@JanStevens
Copy link

This indeed seems to fix it, thanks! 👍

@gaffneyc
Copy link
Contributor

gaffneyc commented Mar 4, 2019

Does the alternate version of this (with just the folder) have the same problem of files clobbering each other? Are the URLs you're trying to simplify / prettify human generated or system generated?

@gauravtiwari
Copy link
Member Author

Does the alternate version of this (with just the folder) have the same problem of files clobbering each other?

It will preserve the containing folder but not the whole path and no, the assets won't be clobbered together.

Are the URLs you're trying to simplify / prettify human generated or system generated?

This comes from [path] option supplied in the name, [path] includes the full relative path of the included asset, which works well but not for assets outside of the source path.

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.

Media assets ignore folder structure
4 participants