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

Need includePattern as option #62

Open
morgs32 opened this issue Jun 5, 2019 · 11 comments
Open

Need includePattern as option #62

morgs32 opened this issue Jun 5, 2019 · 11 comments

Comments

@morgs32
Copy link

morgs32 commented Jun 5, 2019

My issue is that we want to use this babel transform just for icons that we want to style - inline icons that should have the same color as surrounding text, etc.

On the other hand, we'd like to use webpack's url-loader to handle all larger "illustration" SVGs. We would rather put those in img tags and let them be cached by the browser and not have them in our javascript bundle.

To do so, an includePattern could be used that would only use this babel transform on (for example):

  • *.icon.svg
  • */icons/*.svg

Thoughts? I'm about to make a PR.

@ljharb
Copy link
Collaborator

ljharb commented Jun 5, 2019

That seems like something that babel itself can handle in the config?

@morgs32
Copy link
Author

morgs32 commented Jun 6, 2019

This is a nonissue if you do <img src={require(...)} /> because of t.isVariableDeclarator(path.parent) here.

I could close this @ljharb but would love to get some idea of how you mean to configure the babel config.

If you DID want to declare a variable and require a svg that you wanted to be handled by file-loader, how would you do it?
Assuming my webpack already has a rule to watch for .svg and use file-loader.
I was thinking this might work, but it doesn't:

{
  "plugins": [

  ],
  "overrides": [
    {
      "test": "*.icon.svg",
      "plugins": [
        "inline-react-svg"
      ]
    }
  ]
}

@ljharb
Copy link
Collaborator

ljharb commented Jun 6, 2019

I'm not sure what you mean; you're saying if you inline the require, you bypass the plugin?

@morgs32
Copy link
Author

morgs32 commented Jun 6, 2019

Exactly. But also out of pure curiosity I tried the overrides configuration in my babelrc but can't get it to work. Any ideas?

@ljharb
Copy link
Collaborator

ljharb commented Jun 12, 2019

So, fwiw, #28 should have covered that - meaning, it's a bug that it's not transforming the inline require.

As for overrides; i'd jump into the babel slack and ask about it? If you can get that working, that'd be much more efficient than adding an option to this plugin.

@morgs32
Copy link
Author

morgs32 commented Jun 17, 2019

Ok I've taken my overrides question to the babel slack group. Hopefully someone there can fill me in how this plugin gets applied even though webpack is only applied babel-loader to my .js files.

Meanwhile, I want to double check that you think an inline require should be transformed. How would that look in application?

@morgs32
Copy link
Author

morgs32 commented Jun 17, 2019

@knoid I thought I'd cc you in regards to the inline require behavior.

@morgs32
Copy link
Author

morgs32 commented Jun 17, 2019

Ok just discussed this in babel slack

The overrides in babel won't apply because I can't test on svg files. Babel is only applied to javascript files and it transforms the import Svg from './some.svg' in the JS file itself. So arguably, an option on the plugin like includePattern is the only way (that's the conclusion we came to in slack)??

@ljharb
Copy link
Collaborator

ljharb commented Jun 17, 2019

Makes sense.

@kg-currenxie
Copy link

+1
I have a case where I want to have different svgo options based on a test pattern.

@Himanshuub2
Copy link

Hey Anyone working on this issue ? I am also facing similar issue wherein i want only specific set of svg's to be converted into ReactComponent . includeOptions would be great to have, in the meantime how can i ignore all paths , except certian folder with ignorePattern ? so that only required folder is included ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants