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

fix: react-native-windows implementation for new architecture #2527

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

acoates-ms
Copy link

Summary

There are two main things going on with the PR. Both involve a reworking of the new arch implementation of rn-svg for windows.
The current implementation attempts to implement a svg renderer from scratch. There are numerous edge cases that it wasn't handling correctly, and the performance had some serious issues. This implementation switches to use the svg rendering path built into Direct2D. This brings significant performance improvements.

The 2nd issue is there have been various breaking changes in react-native-windows for how new arch native components are implemented. This brings the rn-svg implementation in line with those latest changes.

Test Plan

Primary testing right now is loading up the example app in the repo.
New arch on react-native-windows is still in somewhat early days - so there are not really current users of this code. I am integrating this code into Microsoft Office, where I have tested some scenarios. But we will get expanded testing as we roll out the new arch. I expect there to be some follow-ups as we expand our usage. The version of rn-svg before this PR doesn't build with the latest new arch react-native-windows versions. - So its hard to get worse than that.

What's required for testing (prerequisites)?

What are the steps to reproduce (after prerequisites)?

Compatibility

OS Implemented
iOS N/A
MacOS N/A
Android N/A
Web

Checklist

  • I have tested this on a device and a simulator
  • I added documentation in README.md
  • I updated the typed files (typescript)
  • I added a test for the API in the __tests__ folder

Copy link
Member

@jakex7 jakex7 left a comment

Choose a reason for hiding this comment

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

Thank you @acoates-ms for contribution 🎉 I just left a small suggestion

windows/RNSVG/ReactPackageProvider.cpp Show resolved Hide resolved
@jakex7 jakex7 changed the title [Windows] Fix react-native-windows implementation for new architecture fix: react-native-windows implementation for new architecture Nov 13, 2024
@jakex7
Copy link
Member

jakex7 commented Nov 13, 2024

Hey @acoates-ms, it looks like the CI for fabric-windows-example is failing. Could you look into it? (And don't worry about failing E2E 😅)

@acoates-ms
Copy link
Author

Hey @acoates-ms, it looks like the CI for fabric-windows-example is failing. Could you look into it? (And don't worry about failing E2E 😅)

Oops, I accidently left an Office specific change in the PR, which broke the CI. Should be fixed now. I also brought in latest fixes from react-native-windows to deal with some issues we were having with the new arch and cloning custom props objects.

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