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.

Sign up for GitHub

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

Jump to bottom

Load favicon through html-loader. #428

Merged
merged 7 commits into from
Aug 12, 2016
Merged

Load favicon through html-loader. #428

merged 7 commits into from
Aug 12, 2016

Conversation

andreypopp
Copy link
Contributor

@andreypopp andreypopp commented Aug 12, 2016 •
edited by gaearon
Loading

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:https://localhost:3000
... observe favicon present
npm run build
npm install pushstate-server
./node_modules/.bin/pushstate-server build
open http:https://localhost:9000
... observe favicon present

Fixes #291.

andreypopp
andreypopp reviewed Aug 12, 2016
View reviewed changes
template/index.html
@@ -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.

andreypopp
andreypopp reviewed Aug 12, 2016
View reviewed changes
config/webpack.config.dev.js
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.

This was referenced Aug 28, 2016
Closed
Merged
gaearon added a commit that referenced this pull request Sep 2, 2016
@gaearon
Document #428
4d50072
This was referenced Sep 2, 2016
Merged
Closed
gaearon added a commit that referenced this pull request Sep 2, 2016
@gaearon
Disable implicit serving of the source files (#551)
f1c9584 
* Disable contentBase in development

* Document #428
@gaearon gaearon mentioned this pull request Sep 3, 2016
Closed
stayradiated pushed a commit to stayradiated/create-react-app that referenced this pull request Sep 7, 2016
@andreypopp
Load favicon through html-loader. (facebook#428)
d295979 
* 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
@gaearon
Disable implicit serving of the source files (facebook#551)
ed5f0da 
* Disable contentBase in development

* Document facebook#428
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
@andreypopp @feiqitian
Load favicon through html-loader. (facebook#428)
799131f 
* 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
@gaearon @feiqitian
Disable implicit serving of the source files (facebook#551)
473ef3a 
* 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.
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
0.3.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@andreypopp @gaearon @just-boris