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 file loader public path #11206

Merged
merged 1 commit into from
Nov 4, 2019

Conversation

paulrosenzweig
Copy link
Contributor

My fix in #11174 broke file loading. The presented most obviously as broken fonts.

Here's a history of the issues with this setting:

Situation 1

publicPath: "app/dist/"

file-loader files ended up with this public path duplicated. e.g. "app/dist/app/dist/image.svg". This causes 404's when it tries to load these assets.

Situation 2

publicPath: "/app/dist/"

Everything works! ...unless you host Metabase with a path prefix. In that case everything breaks.

Situation 3

publicPath: "app/dist/"
file-loader's publicPath: ""

The 404's from the first situation disappear, so I assume it works. However, the files are now referenced without "app/dist/" at all! The server returns a 200, but not the file we want.

This PR

publicPath: "app/dist/"
file-loader's publicPath: "./"

This seems to actually load the files correctly! It was taken from this Stack Overflow answer.

@mazameli
Copy link
Contributor

Seems like #11174 also caused a regression that's making this issue appear again for folks: #10657

Just noting this so we remember to close #10657 again once this PR is merged.

@paulrosenzweig
Copy link
Contributor Author

@mazameli I think #11174 did fix #10657. This PR fixes the bug from #11174 (file loader fails), which don't have any issues since we haven't released since that was merged.

@flamber
Copy link
Contributor

flamber commented Oct 24, 2019

@mazameli Just for correction, the non-root regression was caused by #10963 not #11174.

@tlrobinson
Copy link
Contributor

Any thoughts on how we could test this? Unfortunately I think we'd need to run tests in a real browser.

@paulrosenzweig
Copy link
Contributor Author

Any thoughts on how we could test this?

I think this is really a case where I should have been more careful rather than a failure of tests. But, if there were good tests that'd be great! Maybe we could check that there are no 404's on a test page load?

@paulrosenzweig paulrosenzweig merged commit a90fe5a into release-0.33.x Nov 4, 2019
@paulrosenzweig paulrosenzweig deleted the fix-file-loader-public-path branch November 4, 2019 15:44
@flipace
Copy link

flipace commented Nov 5, 2019

@tlrobinson do you have an ETA for the next release?

@paulrosenzweig
Copy link
Contributor Author

@flipace this fix should be out today or tomorrow.

@shemekhe
Copy link

shemekhe commented Aug 9, 2022

It seems the bug hasn't been resolved

@flamber
Copy link
Contributor

flamber commented Aug 9, 2022

@shkhaksar You are commenting on a PR that is almost 3 years old. Create a new issue or use the forum for troubleshooting: https://discourse.metabase.com/

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.

None yet

6 participants