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.

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

Fix transformer-svg-react not finding .svgrrc's #7741

Merged
merged 14 commits into from
Nov 27, 2022
Merged

Fix transformer-svg-react not finding .svgrrc's #7741

merged 14 commits into from
Nov 27, 2022

Conversation

coolreader18
Copy link
Contributor

@coolreader18 coolreader18 commented Feb 21, 2022 •
edited by mischnic
Loading

↪️ Pull Request

componentName was being passed as filePath instead of componentName and as a side effect svgr couldn't properly find a .svgrrc file, if there was one.

Closes #8270

@height
Copy link

height bot commented Feb 21, 2022

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@coolreader18
Copy link
Contributor Author

Though actually, wait, it should probably use parcel's own loadConfig stuff for caching and whatnot. One sec.

@mischnic
Copy link
Member

Can you add a test case for using a svgr config file?

@coolreader18
Copy link
Contributor Author

Alright, added. I'm realizing though that ideally the svgrrc shouldn't be necessary in the first place just to get it to work with preact (though still necessary for other things); would it be best to copy the JSX pragma detection code from JSTransformer, depend on it through package.json, or maybe some other method?

@coolreader18
Copy link
Contributor Author

(And maybe that should be done in a different PR? idk)

@coolreader18 coolreader18 force-pushed the patch-1 branch 2 times, most recently from c984edc to c972bd7 Compare February 22, 2022 02:31
@coolreader18
Copy link
Contributor Author

Is the failure just a fluke?

@mischnic
Copy link
Member

mischnic commented Jul 1, 2022

According to the docs, runtimeConfig: false also disables the SVGO config search. So I think that will have to be loaded in loadConfig as well and then passed to the transform call. (A test for that would also be great).

The assert(file.includes('h("svg"')); assertion is failing on CI, that's not a fluke if it's on every platform.

would it be best to copy the JSX pragma detection code from JSTransformer, depend on it through package.json, or maybe some other method?

Yeah, I think putting that code into some shared package would be the best way. But that's actually a separate problem for another PR.

@mischnic mischnic mentioned this pull request Jul 3, 2022
Closed
@mischnic mischnic mentioned this pull request Aug 8, 2022
Closed
@mischnic mischnic mentioned this pull request Aug 23, 2022
Merged
3 tasks
@mischnic mischnic requested a review from devongovett November 27, 2022 17:07
devongovett
devongovett reviewed Nov 27, 2022
View reviewed changes
packages/transformers/svg-react/src/SvgReactTransformer.js
'svgo.config.json',
'svgo.config.js',
'svgo.config.cjs',
]);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the SVG optimizer only supports svgo.config.*. Maybe we should match that here?

devongovett
devongovett reviewed Nov 27, 2022
View reviewed changes
packages/transformers/svg-react/src/SvgReactTransformer.js Outdated
'svgo.config.cjs',
]);
if (svgrResult) {
let isJavascript = path.extname(svgrResult.filePath) === '.js';
Copy link
Member

Choose a reason for hiding this comment

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

These should also handle .cjs

@devongovett
Copy link
Member

Test appears to be failing...

@mischnic
Copy link
Member

Forgot to rename the .svgorc file in the test 🙈

@mischnic mischnic merged commit ae31c3c into parcel-bundler:v2 Nov 27, 2022
lettertwo added a commit that referenced this pull request Dec 8, 2022
@lettertwo
Merge v2 into lettertwo/parcel-link
6fc5e87 
* upstream/v2:
  Make sure we're compiling on ubuntu-20.04
  v2.8.1
  Add mjs and cjs to resolver extensions (#8667)
  Fix transformer-svg-react not finding .svgrrc's (#7741)
  Fix overriding single export of a `export *` (#8653)
  chore: spelling fix (#8614)
  Parse shortcut icons in web app manifests (#8660)
  Make ts-types transformer work with TS >= 4.8 (#8661)
  Don't retarget dependencies with `*` (#8645)
  fix: remove @parcel/utils dep in @parcel/graph (#8630)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devongovett devongovett devongovett approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

svgo.config.json doesn't work, but svgo.config.js does
3 participants
@coolreader18 @mischnic @devongovett