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(xml): extract tags map to separate entry point for mobile & web #1916

Merged
merged 8 commits into from
Jul 29, 2024

Conversation

joshuayoes
Copy link
Contributor

@joshuayoes joshuayoes commented Nov 9, 2022

Summary

What issues does the pull request solve? Please tag them so that they will get automatically closed once the PR is merged

When utilizing with react-native-web: SvgXml and SvgUri are not exported in the src/ReactNativeSVG.web.ts extension. The logic for SvgXml and SvgUri do not have native dependencies, so they are safe to consume on web.

Issues:

What is the feature? (if applicable)

Add SvgXml and SvgUri as consumable exports for react-native-web

How did you implement the solution?

Extract tags to shapes map into separate tags.tsx file, where native shape elements and web shape elements can be provided to their respective envs.

What areas of the library does it impact?

src directory

Test Plan

Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes UI.

What's required for testing (prerequisites)?

n/a

What are the steps to reproduce (after prerequisites)?

n/a

Compatibility

OS Implemented
iOS
Android

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

@joshuayoes
Copy link
Contributor Author

Another way I considered going about this was creating web.tsx shadows for each exported element in the src/elements directory.
Screen Shot 2022-11-09 at 1 02 29 PM

This would be nice because it would allow src/ReactNativeSVG.web.ts to more closely resemble src/ReactNativeSVG.ts. But I figured it would make the PR noisier with lots of smaller file changes.

@WoLewicki
Copy link
Member

@joshuayoes thanks for the contribution! Do you have knowledge how does it go with the discussion from #1279? Especially security concerns e.g. from #1279 (comment)?

@joshuayoes
Copy link
Contributor Author

@joshuayoes thanks for the contribution! Do you have knowledge how does it go with the discussion from #1279? Especially security concerns e.g. from #1279 (comment)?

Thanks for your response. Taking a deeper look at those issues, I think this PR may not be a good solution for maximal security, which it seems the team has decided to prioritize.

However, it would be nice if there was a clearer error when trying to import these on the web. Perhaps there could be a web export that just throws an error with a link to the PRs?

@WoLewicki
Copy link
Member

Yeah, it seems like a good solution. Maybe even better if there was a link to the #1279 and a note that if you want to use it, then there could be a link to the PRs. Would you mind opening such a PR?

@bohdanprog bohdanprog self-assigned this Jul 23, 2024
src/xml.tsx Outdated Show resolved Hide resolved
src/xml.tsx Outdated Show resolved Hide resolved
src/xml.tsx Outdated Show resolved Hide resolved
@bohdanprog bohdanprog requested a review from jakex7 July 25, 2024 11:43
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.

LGTM, thank you! 🚀

@jakex7 jakex7 merged commit 360e0ee into software-mansion:main Jul 29, 2024
1 check passed
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.

4 participants