Skip to content

Accept version when loading fonts e.g. font-awesome #298

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
Merged
merged 2 commits into from
Jul 30, 2016
Merged

Accept version when loading fonts e.g. font-awesome #298

merged 2 commits into from
Jul 30, 2016

Conversation

@alexzherdev
Copy link
Contributor

@alexzherdev alexzherdev commented Jul 30, 2016 •
edited
Loading

Fixes #295. The version is optional so I didn't extract font extensions into a separate loader.

@ghost ghost added the CLA Signed label Jul 30, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 30, 2016

Shouldn't we allow any query string parameters? They wouldn't have any effect anyway.

@alexzherdev
Copy link
Contributor Author

Ah, I think you're right. Can't think of any harm it could do.

@ghost ghost added the CLA Signed label Jul 30, 2016
@alexzherdev
Copy link
Contributor Author

How about this now?

@gaearon
Copy link
Contributor

gaearon commented Jul 30, 2016

Why not just (?.*)?

@alexzherdev
Copy link
Contributor Author

I thought about this but somehow it seemed safer to enforce the query string structure. Not sure what exactly I was worried about, sorry 😒

gaearon
gaearon reviewed Jul 30, 2016
View reviewed changes
config/webpack.config.dev.js
},
{
test: /\.(jpg|png|gif|eot|svg|ttf|woff|woff2)$/,
test: /\.(jpg|png|gif|eot|svg|ttf|woff|woff2)(\?.*)?$/,
Copy link
Contributor

Choose a reason for hiding this comment

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

Also worth doing for mp4/webm in the loader a few lines below

Copy link
Contributor Author

@alexzherdev alexzherdev Jul 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I'm not sure how else it's used in the wild, only encountered it with fonts. I wonder if other assets like CSS may need this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's relevant for anything that can be referenced in CSS. So CSS itself won't need it but any files will.

Copy link

@sokra sokra Jul 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

relevant for url(font.woff?v=1.3) in some CSS libs. webpack 2 doesn't need this anymore. cc @mxstbr

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does webpack 2 not need this anymore? What am I missing here?

@ghost ghost added the CLA Signed label Jul 30, 2016
@ghost ghost added the CLA Signed label Jul 30, 2016
@gaearon gaearon added this to the 0.3.0 milestone Jul 30, 2016
@gaearon gaearon merged commit c239551 into facebook:master Jul 30, 2016
@gaearon gaearon modified the milestones: 0.2.1, 0.3.0 Aug 1, 2016
@gaearon gaearon mentioned this pull request Aug 1, 2016
Merged
@tsunammis tsunammis mentioned this pull request Aug 17, 2016
Merged
@EnoahNetzach EnoahNetzach mentioned this pull request Mar 5, 2017
Closed
@BradMcDanel BradMcDanel mentioned this pull request Jun 1, 2017
Closed
@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.2.1

Development

Successfully merging this pull request may close these issues.

4 participants

@alexzherdev @gaearon @sokra @mxstbr