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

perf: remove the builtin @rsbuild/plugin-svgr #716

Merged
merged 1 commit into from
Mar 6, 2024
Merged

Conversation

chenjiahan
Copy link
Member

@chenjiahan chenjiahan commented Mar 6, 2024

Summary

Remove the built-in SVGR plugin since Rspress does not rely on this plugin.

The SVGR feature is not mentioned in the documentation, but some users who have used custom React components may rely on this behavior, so this could be a potentially breaking change.

Benefits

The benefits of removing the SVGR plugin are obvious, it can significantly reduce the number of dependencies (-80), and it also allows the use of different SVGR syntax or config, so we will remove it in the next minor version of Rspress.

Screenshot 2024-03-06 at 11 58 29

Migration

Users who have SVGR needs can manually use the @rsbuild/plugin-svgr:

  1. run pnpm add @rsbuild/plugin-svgr -D
  2. register this plugin in rspress.config.ts:
import { pluginSvgr } from '@rsbuild/plugin-svgr';

export default defineConfig({
  builderConfig: {
    plugins: [
      pluginSvgr({
        svgDefaultExport: 'component',
      }),
    ],
  }
});

Checklist

  • I have added changeset via pnpm run change.
  • I have updated the documentation.
  • I have added tests to cover my changes.

Copy link

netlify bot commented Mar 6, 2024

Deploy Preview for aquamarine-blini-95325f ready!

Name Link
🔨 Latest commit 410552d
🔍 Latest deploy log https://app.netlify.com/sites/aquamarine-blini-95325f/deploys/65e7f5386b7bb50008dfab8c
😎 Deploy Preview https://deploy-preview-716--aquamarine-blini-95325f.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 89 (🟢 up 3 from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@chenjiahan chenjiahan marked this pull request as draft March 6, 2024 04:51
@chenjiahan chenjiahan marked this pull request as ready for review March 6, 2024 06:10
@sanyuan0704
Copy link
Contributor

I think the default theme will have svg render problem after executing DOC_DEBUG=true pnpm dev

@sanyuan0704
Copy link
Contributor

cc @10Derozan

@chenjiahan
Copy link
Member Author

I think the default theme will have svg render problem after executing DOC_DEBUG=true pnpm dev

Can you provide more info for this?

@10Derozan
Copy link
Contributor

I think the default theme will have svg render problem after executing DOC_DEBUG=true pnpm dev

Can you provide more info for this?

Debug mode will use source code
https://github.com/web-infra-dev/rspress/blob/main/packages/theme-default/src/node/source-build-plugin.ts#L19

@10Derozan
Copy link
Contributor

Have you test IconWrapper which you suggested?

@chenjiahan
Copy link
Member Author

Ok, I get it.

I can not reproduce the problem because the SVG icons are always resolved to the dist folder now:

image

@chenjiahan chenjiahan mentioned this pull request Mar 6, 2024
3 tasks
@chenjiahan
Copy link
Member Author

Have you test IconWrapper which you suggested?

Implemented in #720.

It works and allows us to use both string SVG or component SVG at the same time.

@sanyuan0704
Copy link
Contributor

Ok, I get it.

I can not reproduce the problem because the SVG icons are always resolved to the dist folder now:

image

Get it

@sanyuan0704 sanyuan0704 merged commit 7d1103c into main Mar 6, 2024
9 checks passed
@sanyuan0704 sanyuan0704 deleted the remove_svgr_0306 branch March 6, 2024 07:23
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