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

Load favicon through html-loader. #428

Merged
merged 7 commits into from
Aug 12, 2016
Merged

Conversation

andreypopp
Copy link
Contributor

@andreypopp andreypopp commented Aug 12, 2016

Migration Instructions

Now favicon.ico is not treated specially anymore.
If you use it, move it to src and add the following line to <head> in your HTML:

<link rel="shortcut icon" href="./src/favicon.ico">

Use html-loader to load favicon.

Test plan:

npm run create-react-app my-app
cd my-app
npm start
open http://localhost:3000
... observe favicon present
npm run build
npm install pushstate-server
./node_modules/.bin/pushstate-server build
open http://localhost:9000
... observe favicon present

Fixes #291.

@@ -3,6 +3,7 @@
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1">
<link rel="shortcut icon" href="${require('./src/favicon.ico')}">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

webpack-ism

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a way to make html-loader work just on <link href="./src/favicon.js"> but I don't know how.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to find it :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? require("./src/favicon.js") seems less magical and also consistent with other webpack-isms used (require("./index.css")).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just a weird thing to explain. Imports are natural in JS but odd in HTML. Also since we use ES6 some people won't even know what require is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@ghost ghost added the CLA Signed label Aug 12, 2016
@andreypopp
Copy link
Contributor Author

Also, I don't have any idea how all of this works... Seems like html-webpack-plugin runs html through html-loader?interpolate.

test: /\.html/,
loader: 'html',
query: {
attrs: ['link:href'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be extended with other tag:attribute combinations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh nice. Can you think up any other common combinations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't bother adding more at the moment but rather serve on per-request basis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think img:src is out of scope.

@gaearon gaearon added this to the 0.3.0 milestone Aug 12, 2016
@ghost ghost added the CLA Signed label Aug 12, 2016
@gaearon gaearon merged commit 60178ac into facebook:master Aug 12, 2016
@gaearon
Copy link
Contributor

gaearon commented Aug 12, 2016

👍 Nice job

@just-boris
Copy link
Contributor

As you already have html-webpack-plugin, you can pass favicon option to it, instead of defining a lot of loaders.

@andreypopp Why you didn't do that?

@gaearon
Copy link
Contributor

gaearon commented Aug 14, 2016

@just-boris This was exactly how it worked before. The intention here is not to solve the favico case but solve the generic problem of “how do I reference image (or other) files from index.html”? The previous solution only worked for favicons, this solution will also work for apple touch icons etc, is more generic, and explicit.

gaearon added a commit that referenced this pull request Sep 2, 2016
gaearon added a commit that referenced this pull request Sep 2, 2016
* Disable contentBase in development

* Document #428
stayradiated pushed a commit to stayradiated/create-react-app that referenced this pull request Sep 7, 2016
* Load favicon through html-loader.

Fixes facebook#291.

* Add test for *.ico in e2e test suite

* Configure html-loader to process <link href="...">

* Address feedback on html-loader inclusion.

* Place favicon.ico at the root of the build dir

* Make comment style consistent between prod and dev webpack configs

* Fix html-loader config in dev mode
stayradiated pushed a commit to stayradiated/create-react-app that referenced this pull request Sep 7, 2016
* Disable contentBase in development

* Document facebook#428
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
* Load favicon through html-loader.

Fixes facebook#291.

* Add test for *.ico in e2e test suite

* Configure html-loader to process <link href="...">

* Address feedback on html-loader inclusion.

* Place favicon.ico at the root of the build dir

* Make comment style consistent between prod and dev webpack configs

* Fix html-loader config in dev mode
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
* Disable contentBase in development

* Document facebook#428
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants