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 [query] in interpolateName #154

Closed
wants to merge 4 commits into from
Closed

Support [query] in interpolateName #154

wants to merge 4 commits into from

Conversation

pajter
Copy link

@pajter pajter commented Sep 28, 2019

This PR adds support for [query] in the template of interpolateName.

Some background: I want to load SVG files in CSS but pass query parameters to them to influence SVG properties such as fill etc. After some digging I found out that file-loader uses the interpolateName function in this lib, and that there's no way to use [query] in the template. Seemed to me like this would be nice to have and easy enough to support.

I'd love to get your feedback on this. Please let me know if there's anything I missed!

@jsf-clabot
Copy link

jsf-clabot commented Sep 28, 2019

CLA assistant check
All committers have signed the CLA.

@pajter
Copy link
Author

pajter commented Sep 28, 2019

Seems that yarn is preventing tests in Node 4 from running.

@@ -52,6 +52,9 @@ function interpolateName(loaderContext, name, options) {
let basename = 'file';
let directory = '';
let folder = '';
const query = loaderContext.resourceQuery
? loaderContext.resourceQuery.substr(1)
: '';
Copy link
Member

Choose a reason for hiding this comment

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

I think we can solve this in easy way, like:

const queryStringIdx = targetFile.indexOf('?');
let query = ''; 
if (queryStringIdx >= 0) {
  query= targetFile.slice(queryStringIdx);
}  

without

Copy link
Author

Choose a reason for hiding this comment

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

I'm not quite sure if I understood correctly. As far as I understand, the resourcePath and resourceQuery are two separate entities. I can't split the path by ?, but I was trying to remove the leading ? character. I updated the code to be a bit more verbose. I hope this is what you meant.

Copy link
Member

Choose a reason for hiding this comment

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

Logic is simple, if name contains ?foo=bar we interpreted this as [query]

Copy link
Author

Choose a reason for hiding this comment

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

Could you elaborate on which name you mean? I can only get ?foo=bar from loaderContext.resourceQuery. I feel like I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

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

name - second argument for interpolateName

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Good job, one note

@alexander-akait
Copy link
Member

Just for information, we move loader-utils logic inside webpack@5, so this module will be deprecated

@alexander-akait
Copy link
Member

Need fix CI Problem

@pajter
Copy link
Author

pajter commented Sep 28, 2019

I found an issue on the error that Yarn is throwing in CI: yarnpkg/yarn#6619 (comment)

@pajter
Copy link
Author

pajter commented Oct 3, 2019

@evilebottnawi Anything I can help with?

I can update package.json to use npm run instead of yarn, which will fix the Yarn error in Node 4. But then eslint will error because it uses destructuring syntax. Then you'd probably have to downgrade ESLint to support Node 4, or skip the lint entirely in Node 4.

For what it's worth, I can run the Jest test suite in Node 4 and everything passes.

Screenshot 2019-10-03 at 21 42 12

@pajter
Copy link
Author

pajter commented Oct 3, 2019

Actually, I checked a bit better and see that ESLint isn't even running in CI. It's just the yarn commands that are failing in Node 4. It will work with npm, but I'm not sure if you'd want to change that or wait for Yarn to fix its issue. Doesn't seem to have a lot of activity...

@alexander-akait
Copy link
Member

@pajter ignore problem with node@4

@pajter
Copy link
Author

pajter commented Oct 30, 2019

Is this okay to merge then?

@alexander-akait
Copy link
Member

Done in master, sorry for delay, really sorry

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.

3 participants