Upgrade @svgr/webpack to 4.1.0

[alaycock]

I ran into an issue when upgrading to create-react-app 2 because @svgr/core@2.4.1 (a dependency of @svgr/webpack) had prettier as a dependency, which led to two different version of prettier being installed, which ended up causing linting issues when the two different versions of prettier ran (through yarn eslint vs yarn prettier). More details of my issue can be found here: prettier/eslint-plugin-prettier#110

As of @svgr/core@4.0.0, prettier and svgo are no longer dependencies, and using it will prevent them from being installed, which would hopefully prevent prettier version conflicts in the future.

Neither prettier and svgo were being used in create-react-app anyway, so having them removed just removes a bit of node_modules bloat without cutting functionality.

To test, run create react app, and try out importing a SVG as a component with import { ReactComponent as Logo } from "./logo.svg";, and it should continue to work.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[iansu]

This was on my todo list so thanks for taking care if it!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[alaycock]

I fixed the failing tests on Travis, but not sure why appveyor is failing, looks like it's timing out. I added the babel packages to resolve the failing tests, but if there's a more appropriate way to add to the project, I'm all ears.

[iansu]

Appveyor had been broken for a while so don't worry about that.

I'm more concerned about the fact that we have to add all of these babel dependencies. It's also weird that they seem to only be required in ejected projects. @neoziro do you have any idea why this might be?

[gregberge]

@iansu I don’t know why they are required, probably babel sub dependencies. But I think it is lighter than before.

[gregberge]

(SVGO is included in @svgr/webpack and I think it should be part of cra). It could reduce the bundle size.

[gregberge]

In your PR, for now it is included.

[alaycock]

That looks like a mistake on my part, I misunderstood how svgo was being included. Currently it appears to be intentionally left out, do you have a reason for excluding it @iansu?

I updated the PR to remove prettier and svgo to feature-match with the existing implementation.

Also trying to see if I can get away without a couple extra dependencies, since CI seems to be acting differently than my machine.

[alaycock]

Looks like SVGO was disabled intentionally., so I will leave it disabled unless there is a good reason to change it.

Removing prettier and SVGO seems to have required another dependency for the kitchensink-eject tests to pass. Still not sure why that is the case or how to resolve that, so any additional help would be appreciated.

[alaycock]

I think I should be good to go now. My issue seems to have been resolved by merging in master which brought in "@babel/preset-env": "7.1.6", which seems to have resolved the conflict between versions being used by @svgr/webpack and this project.

[gregberge]

@alaycock OK good, please let me fix this one and release before fixing the version.

[alaycock]

@neoziro I don't think that's necessary after all. Updating the version of preset-env seems to have resolved my problem. Thanks for being accommodating though.

[gregberge]

I think it is, the way plugin are imported could create conflict. If the user install babel + create-react-app or if he ejects. I prefer to have it fixed. The fix will be published in 5 minutes.

[gregberge]

@alaycock I think making config static and using createConfigItem could improve perf too! https://github.com/smooth-code/svgr/pull/240/files

[gregberge]

@alaycock published as v4.0.4

[iansu]

I think we can remove -prettier from the webpack loader strings since it's no longer included. At least that's my understanding. We still want to leave SVGO out. Also there are some new tests in babel-plugin-named-asset-import that you'll have to update.

[alaycock]

You're right @iansu, I made that change. It looks like svgo could be disabled with -svgo, or by preventing it from loading it as a plugin with plugins[]='@svgr/plugin-jsx'. The docs don't indicate anything about enabling/disabling plugins, so I went with -svgo because that's what was used previously. Not sure what the preferred method would be though. The performance impact of loading the plugin, but not using it, is probably insignificant.

[gregberge]

@alaycock I think -svgo is fine, avoid the loading of the plugin is not significant in term of perf. It is easier to stick with ˋ-svgo` and probably more future proof.

I published a v4.1.0, the only change that impacts you is an upgrade of unified / rehype. It is marked as minor because of the new parcel plugin.

[kopax]

Hi, any update on this? I am experiencing an issue with the old version of @svgr/webpack, does anybody have a temporary workaround to offer? Thanks.

[alaycock]

@kopax I think this is good to go unless there's any more feedback.

[kopax]

Ok. Maybe they should merge this and release a new CRA version for testing. Do you know how I can test this easely? Maybe we can replace react-script by a fork dist version on npm?

[stale]

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

[alaycock]

@iansu, is there anything I should be doing to get this ready to merge? It sounds like some other PRs are waiting on this one.

[iansu]

These build failures seem unrelated. We're currently trying to fix them: #6165. Once that's done we should be good to merge this.

[ianschmitz]

#6165 is in and we're all green. Let's do this!

[iansu]

Thanks!