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

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

Merged
merged 2 commits into from
Jul 30, 2016

Conversation

alexzherdev
Copy link
Contributor

@alexzherdev alexzherdev commented Jul 30, 2016

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 😒

@@ -73,7 +73,7 @@ module.exports = {
loader: 'json'
},
{
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

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

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
@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.

4 participants