Skip to content

Revert "Remove context from file loader (#1914)" #1950

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

Closed
wants to merge 2 commits into from
Closed

Revert "Remove context from file loader (#1914)" #1950

wants to merge 2 commits into from

Conversation

gaffneyc
Copy link
Contributor

This reverts changes to how assets are managed introduced in RC6. The
changes themselves were introduced to fix #1887 but I couldn't draw a
line between the issue and how these changes were meant to address it.
Additionally, they didn't appear to help the user who made the initial
request and caused a host of other problems.

This appears to be the potential cause for #1938, #1915, #1947, and #1922.

This reverts commit 35ec3ae.

@gaffneyc gaffneyc mentioned this pull request Feb 18, 2019
@jakeNiemiec
Copy link
Member

remember to update the changelog too

@gaffneyc
Copy link
Contributor Author

I wasn't sure what to put into the CHANGELOG. Since RC6 and RC7 have both been released it doesn't make sense to remove them from the changelog completely.

@jakeNiemiec
Copy link
Member

jakeNiemiec commented Feb 18, 2019

Note that I just found: a74193b#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR24

But I still think that this is a poor default for new users. @marcohamersma, can you comment on whether adding context this way would fix your deploy?

@marcohamersma
Copy link

Hey, thanks for looking in to this.
At the moment I can't seem to recreate the original issue I was having on my local machine, so I also haven't been able to verify that this PR or the fix Jake mentioned worked.

I'm a bit confused about that and I'd like to look in to it in detail, but I'm a bit swamped the next few days :( Hope to look in to it when I can.

@guilleiguaran
Copy link
Member

guilleiguaran commented Feb 25, 2019

@gaffneyc wdyt about keeping the assets inside of media but preserving the folder structure?

e.g:

/app/javascripts/images/projects/icon.svg -> packs/media/images/projects/icon-hash.svg
/app/javascripts/images/users/icon.svg -> packs/media/images/users/icon-hash.svg

And the helper for images then can be used in this way:

image_pack_tag "projects/icon.svg"

@gaffneyc
Copy link
Contributor Author

gaffneyc commented Feb 26, 2019

@guilleiguaran I think a discussion on using media as a base directory is out of scope of this change. Things, as they are today, are broken for a bunch of folks and we need to get that fixed. A separate PR to reintroduce media would give a chance to discuss it rather than what ended up being a very rushed PR without an obvious rationale.

This reverts changes to how assets are managed introduced in RC6. The
changes themselves were introduced to fix #1887 but I couldn't draw a
line between the issue and how these changes were meant to address it.
Additionally, they didn't appear to help the user who made the initial
request and caused a host of other problems.

This appears to be the potential cause for #1938, #1845, #1915, #1947,
and #1922.

This reverts commit 35ec3ae.
@jakeNiemiec
Copy link
Member

@guilleiguaran See #1903 (comment) for more context around this PR.

@guilleiguaran
Copy link
Member

guilleiguaran commented Feb 26, 2019

Looking to the commit being reverted I can see that it introduced two changes:

  1. Remove context from file loader in favour of simpler paths
  2. Namespaces for compiled packs in the public directory

I understand (1) is breaking many existing configurations, but we need to revert (2) as well?

My proposal is revert (1) but preserving (2) and then we can change file-loader to use media namespace but with paths preserved so, the defaults for file-loader would be:

options: {
  name: 'media/[path][name]-[hash].[ext]',
  context: join(sourcePath)
}

@gaffneyc
Copy link
Contributor Author

@guilleiguaran Is it worth holding off fixing 1 while there is a discussion and eventual change for 2? I would say no.

I propose we get this merged then make another commit or PR to reintroduce media (though I don't understand the rationale or problem that it's trying to solve).

@gaffneyc
Copy link
Contributor Author

@guilleiguaran See #1964 with a version that maintains the change to include media

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.

resolved_paths seems not working for images
4 participants