-
Notifications
You must be signed in to change notification settings - Fork 71
Bumps svgr from v5 to v8 #3163
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
base: main
Are you sure you want to change the base?
Bumps svgr from v5 to v8 #3163
Conversation
🦋 Changeset detectedLatest commit: 71635dc The changes in this PR will be included in the next version bump. This PR includes changesets to release 72 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR upgrades the SVGR dependency from version 5 to version 8, which requires significant refactoring of the icon generation code due to breaking API changes between these major versions.
- Removes the custom SVGR template system and replaces it with inline string replacement
- Updates the SVGR API calls to use the new v8 interface with
transform()
instead of the default export - Maintains the same LeafyGreen component wrapper functionality through post-processing
Reviewed Changes
Copilot reviewed 4 out of 187 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
packages/icon/scripts/prebuild/svgrTemplate.ts | Completely removed - custom template no longer needed with new approach |
packages/icon/scripts/prebuild/index.ts | Refactored to use SVGR v8 API and inline string replacement for component customization |
packages/icon/package.json | Updated SVGR dependency from ^5.3.1 to ^8.1.0 |
.changeset/itchy-friends-change.md | Added changeset documenting the SVGR version bump |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
const customizedSVGR = processedSVGR.replace( | ||
/import \* as React from "react";\nimport type \{ SVGProps \} from "react";\nconst (\w+) = \(props: SVGProps<SVGSVGElement>\) => (.*?);\nexport default \1;/s, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This complex regex pattern is fragile and depends on exact formatting from SVGR output. Consider using an AST-based approach or at minimum, break this into smaller, more readable regex patterns with comments explaining each part.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
Size Change: +41.9 kB (+2.64%) Total Size: 1.63 MB
ℹ️ View Unchanged
|
|
||
// Post-process to add custom LeafyGreen wrapper | ||
const customizedSVGR = processedSVGR.replace( | ||
/import \* as React from "react";\nimport type \{ SVGProps \} from "react";\nconst (\w+) = \(props: SVGProps<SVGSVGElement>\) => (.*?);\nexport default \1;/s, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing svgrTemplate.ts
and hard-coding it inline here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SVGR 8 still takes a template
prop. (https://react-svgr.com/docs/options/#template) IMO we should be using this instead of hard-coding a complex regex replacer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
|
||
// Note: must use `require` since svgrrc is a CommonJS module | ||
// @ts-ignore - svgrrc is not typed | ||
const svgrrc = await require('../../.svgrrc.js'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we rm svgrrc.js
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're not using .svgrrc.js
, we should remove it
componentName: file.name, | ||
plugins: ['@svgr/plugin-jsx'], | ||
typescript: true, | ||
jsx: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we extract this config into either a separate file, or at least a constant?
); | ||
|
||
// Apply custom LeafyGreen wrapper using template | ||
const customizedSVGR = applyLeafyGreenTemplate(processedSVGR, file.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this just moves the brittle string replacement to a separate file. We should use the transform Config.template
property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed Cursor has difficulty doing this. There's a weird bug/quirk in the tpl
function where it doesn't like when you concat words after interpolated var names.
If you tell cursor to do that it works:
e.g. instead of ${variables.componentName}Props)
use ${variables.componentName + "Props)"}
✍️ Proposed changes
🎟 Jira ticket: Name of ticket
✅ Checklist
For new components
For bug fixes, new features & breaking changes
pnpm changeset
and documented my changes🧪 How to test changes