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

Support for fonts ttf, eot and woff. problem with the way that URLs a re resolved by Chrome when they're parsed from a dynamically loaded CSS blob #125

Merged

Conversation

iLeonelPerea
Copy link
Contributor

here the solution that I implemented for the issue with Chrome http://stackoverflow.com/questions/34133808/webpack-ots-parsing-error-loading-fonts

…re resolved by Chrome when they're parsed from a dynamically loaded CSS blob
@coryhouse
Copy link
Owner

Nice find! Thoughts:

  1. The publicPath change should only be required for dev since a separate CSS file is generated in prod, which avoids this issue, correct?

  2. A comment next to the publicPath setting referencing this the reason it's useful might be helpful.

  3. I'd suggest tweaking the loader settings for eot, ttf, etc to the following.

    {test: /.eot(?v=\d+.\d+.\d+)?$/, loader: "file"},
    {test: /.(woff|woff2)$/, loader: "url?prefix=font/&limit=5000"},
    {test: /.ttf(?v=\d+.\d+.\d+)?$/, loader: "url?limit=10000&mimetype=application/octet-stream"},
    {test: /.svg(?v=\d+.\d+.\d+)?$/, loader: "url?limit=10000&mimetype=image/svg+xml"}

Note the explicit mimetype and use of url loader limits for efficiency.

@iLeonelPerea
Copy link
Contributor Author

tnks @coryhouse I did the modification to the dev enviroment and add the url-loader dependency.

I think is ready.. 💃

@iLeonelPerea
Copy link
Contributor Author

@coryhouse fix merge conflicts..

you think is ok now?

@coryhouse coryhouse merged commit d7eb07b into coryhouse:master May 5, 2016
@coryhouse
Copy link
Owner

Thanks for the PR and resolving the merge conflicts! 👍

@coryhouse
Copy link
Owner

@iLeonelPerea Do you have a simple example we can use to reproduce this?

We may need to change the way we implemented this fix to resolve #205

@@ -17,7 +17,7 @@ export default {
target: 'web', // necessary per https://webpack.github.io/docs/testing.html#compile-and-test
output: {
path: __dirname + '/dist', // Note: Physical files are only output by the production build task `npm run build`.
publicPath: '/',
publicPath: 'http://localhost:3000/', // Use absolute paths to avoid the way that URLs are resolved by Chrome when they're parsed from a dynamically loaded CSS blob. Note: Only necessary in Dev.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought for was dynamic, if you are already running you could be on 3002. Wouldn't there be conflicts with this? May want to consider using process.env.port to cover that scenario.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent point @kwelch. There's a bug open right now related to this: #181

@kwelch
Copy link
Collaborator

kwelch commented Jul 20, 2016

I know how to reproduce this. It because an error with bootstrap and using glyphicon. I never attempted to fix since it only affected local dev environment. Basically, with public path reverted and bootstrap in place it cannot find the font to resolve the image.

This PR does properly fix the issue, but it is causing a few others.

I have a couple ideas, I will give them a try and report back.

@kwelch
Copy link
Collaborator

kwelch commented Jul 20, 2016

So... I don't want to throw in the towel, but these really don't play nice together.

I tried a few different things but still have not had any luck to get both glyphicons and external access (#205).

Those are my notes, hopefully someone has an idea I have not already exhausted.

Other things to consider:

  • Bootstrap 4.0 is in alpha, and it officially drops glyphicon support (http://v4-alpha.getbootstrap.com/migration/)
  • Slingshot currently does not implement bootstrap
  • This issue only affects dev server, I am running a project that is affected by this missing font issue and once built/deployed glyphicons work fine

@kwelch
Copy link
Collaborator

kwelch commented Jul 20, 2016

This looks promising.

webpack-contrib/style-loader#96 (comment)

It is late will try again tomorrow.

@coryhouse
Copy link
Owner

@kwelch Thanks for all the digging on this! A tricky one indeed. And yes, that last idea looks promising - it would be nice to avoid the public path complexity altogether.

@kwelch
Copy link
Collaborator

kwelch commented Jul 20, 2016

Agreed, the true fix would be for the loaders to support relative path.

I am not a huge fan of the hacky way mentioned in the bug thread, and first attempt last night was fruitless.

@kwelch
Copy link
Collaborator

kwelch commented Jul 21, 2016

Submitted. PR in this using the entry method from the style-loader thread

tpetillon added a commit to tpetillon/roofmapper-client that referenced this pull request Aug 3, 2016
A public path is added in webpack.config.js because of a problem with font loading.
cf. coryhouse/react-slingshot#125
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.

4 participants