-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Not rendering local images #1306
Comments
I think the approach in react-native-svg-uri is similar to something like this: import React from 'react';
import {SvgCssUri} from 'react-native-svg';
import resolveAssetSource from 'react-native/Libraries/Image/resolveAssetSource';
const test = require('./test.svg');
const svg = resolveAssetSource(test);
export default () => <SvgCssUri uri={svg.uri} width="100%" height="100%" />; And won't work on android in release builds, so I don't see the point of adding partial and relatively deceptive support, when there exists a working and better performing solution to it already. |
@msand, thank you for your quick response. I apologize as I think there are some misunderstandings. I know how to render SVG files using What I am asking here, is that most new users would expect a behavior closer to If writing a wrapper to consider if a file is local is too much, then maybe pasting your second suggestions to the documentation may be helpful. For example, adding "when using local svg files, please use SvgCssUri instead, example follows after this sentence..." under the Image category. |
What do you mean by being backed by the react-native team? I'm not affiliated with facebook in any way, and receive no compensation, nor any significant support I can remember from the react-native team. The issue seems like it belongs in the react-native repo if anywhere, I don't think it's possible to resolve the issue of requiring local .svg files in android release builds inside this library, or at least have no clue how to. Feel free to suggest ways to fix it / make a pr. |
Alternatively it might belong in the metro repo. |
And seems like new users wouldn't even know about react-native-svg-uri, and making assumption about what they expect to work seems a bit premature. Especially with regards to CommonJS style require calls, I see no reason to expect them to do anything but cause an exception, as the files don't contain CommonJS modules. |
I don't think it makes sense to expose the require style usage, as it's broken on android, nor to depend on importing internals which aren't exposed by the main entry point, why the insistence on this syntax/setup? Is the current approach using react-native-svg-transformer insufficient or lacking somehow? It should at least perform much better than using strings of xml, parsing that, resolving css, and only then start creating a vdom, all earlier steps are done at build time with the transformer approach. Only use-case I see for xml strings is if you're using something that produces them dynamically, i.e. when there is no other feasible option. |
If you really want to do this, I'd recommend you define a component yourself, something like this: import React from 'react';
import {SvgCssUri} from 'react-native-svg';
import resolveAssetSource from 'react-native/Libraries/Image/resolveAssetSource';
// NB Broken on android in release builds.
export function RequireLocalSvgIosOnly({asset, ...rest}) {
return <SvgCssUri uri={resolveAssetSource(asset).uri} {...rest} />;
} App.js import React from 'react';
import {RequireLocalSvgIosOnly} from './RequireLocalSvgIosOnly';
export default () => <RequireLocalSvgIosOnly asset={require('./test.svg')} />; |
I found there's a library specialised on this specific issue: react-native-local-resource, so you can do like this: LocalSvg.js import React, {useState, useEffect} from 'react';
import {SvgCss} from 'react-native-svg';
import loadLocalResource from 'react-native-local-resource';
export function LocalSvg({asset, ...rest}) {
const [xml, setXml] = useState(null);
useEffect(() => {
loadLocalResource(asset).then(setXml);
}, [asset]);
return <SvgCss xml={xml} {...rest} />;
} App.js import React from 'react';
import {LocalSvg} from './LocalSvg';
export default () => <LocalSvg asset={require('./test.svgx')} />; But, you have to configure it in a similar way to transformer: https://github.com/IgorBelyayev/React-Native-Local-Resource#usage And, it'll have much worse performance in comparison. It adds several layers of processing at run-time, and in comparison, transformer can skip several parts of the processing and move it to build time. Use this pattern if you wish, but I can't figure why you would. |
And, you have to use a non-standard file-extension, e.g. svgx as in this example. |
If you think this pattern is valuable, feel free to make it into a npm package under any license you want, consider it under Unlicense for my part. Closing now. Feel free to ask more questions / comment. |
Actually https://github.com/feats/babel-plugin-inline-import as documented in https://github.com/react-native-community/react-native-svg#use-with-svg-files will give you better performance than this, at least two context switches less between js and native. But still, the transformer approach is much better, it also allows you to run the content thru svgo, often making the rendering simpler and more performant in the process, as the original svg content might be overly complex for what it actually does. |
Alternatively, if you want to use svg files with the Image component from react-native and react-native-svg, then you can look into rasterizing them using https://github.com/aeirola/react-native-svg-asset-plugin |
If you find the current documentation lacking, it would be very appreciated if you could make a pr with improvement suggestions. |
I've made a pr to the local-resource package in an attempt to optimize it: IgorBelyayev/React-Native-Local-Resource#11 May be that in many cases, this pattern will give "good enough" performance, but i'd still prioritise both battery energy and render time / user perceived latency by using the transformer approach, it'll quite certainly give wider budgets and thus handle more complex content with acceptable performance, especially on low-end devices, or when low on resources in general. |
Perhaps should fix the support for plain .svg files, a bit strange to have it mapped when it's not working, and annoying to have wrong/non-standard extension. Seems should patch react-native, metro, and most importantly react-native-community/cli diff --git a/node_modules/@react-native-community/cli/build/commands/bundle/assetPathUtils.js b/node_modules/@react-native-community/cli/build/commands/bundle/assetPathUtils.js
index 78744e7..77b83e3 100644
--- a/node_modules/@react-native-community/cli/build/commands/bundle/assetPathUtils.js
+++ b/node_modules/@react-native-community/cli/build/commands/bundle/assetPathUtils.js
@@ -43,7 +43,7 @@ function getAndroidAssetSuffix(scale) {
} // See https://developer.android.com/guide/topics/resources/drawable-resource.html
-const drawableFileTypes = new Set(['gif', 'jpeg', 'jpg', 'png', 'svg', 'webp', 'xml']);
+const drawableFileTypes = new Set(['gif', 'jpeg', 'jpg', 'png', /*'svg',*/ 'webp', 'xml']);
function getAndroidResourceFolderName(asset, scale) {
if (!drawableFileTypes.has(asset.type)) {
diff --git a/node_modules/@react-native-community/cli/build/commands/server/external/xsel b/node_modules/@react-native-community/cli/build/commands/server/external/xsel
old mode 100644
new mode 100755
diff --git a/node_modules/react-native/Libraries/Image/assetPathUtils.js b/node_modules/react-native/Libraries/Image/assetPathUtils.js
index 6281c95..71da47a 100644
--- a/node_modules/react-native/Libraries/Image/assetPathUtils.js
+++ b/node_modules/react-native/Libraries/Image/assetPathUtils.js
@@ -39,7 +39,7 @@ const drawableFileTypes = new Set([
'jpeg',
'jpg',
'png',
- 'svg',
+//'svg',
'webp',
'xml',
]);
diff --git a/node_modules/metro/src/Bundler/util.js b/node_modules/metro/src/Bundler/util.js
index a50de51..9e95f1f 100644
--- a/node_modules/metro/src/Bundler/util.js
+++ b/node_modules/metro/src/Bundler/util.js
@@ -144,7 +144,7 @@ function generateRemoteAssetCodeFileAst(
function isAssetTypeAnImage(type) {
return (
- ["png", "jpg", "jpeg", "bmp", "gif", "webp", "psd", "svg", "tiff"].indexOf(
+ ["png", "jpg", "jpeg", "bmp", "gif", "webp", "psd", /*"svg",*/ "tiff"].indexOf(
type
) !== -1
);
|
@msand, hi, sorry for the delay, it is weekend over here. I read somewhere that Just a personal opinion that I think it is better we work to make this the repo (which is the first result) instead of having to search around. The number of similar Github and StackOverflow issues made me decide to ask you to change the design pattern. Fragmentation is a sin (I am looking at you, Linux distros) and we also have the problem of the curse of inactive leaders, causing loss of productivity. For example, I am not sure if adding a conditional wrapper and a fixed space (installing other libraries) to check if the link is local or remote would have a noticeable negative performance impact, but it sure will have a little. I understand that svg drawings already require considerable performance and I respect your decision. But it is a good thing that this issue exists now because somebody might google it up and find this issue, which contains links and the status of svg rendering stored locally, saving them time. Closing this issue because maintainer decided that the design pattern does not fit the repo's vision. |
Well, given that the patch to cli gets released, I consider adding the LocalSvg component valuable, as it can actually work on both android and ios, with plain .svg extension, in both debug and release mode. Alternatively it can be made to work immediately already using patch-package and the patches I've provided in the previous message. Ideally I'd see that the users of this library would use the most energy and time efficient method to render their vector graphics as a baseline. But see that it can also make sense for many projects to completely ignore rendering performance for various reasons, and just get things done asap. As this approach (given new cli release) can allow using .svg files by only adding react-native-svg, and no futher configuration, I consider it a valuable addition to the api, given clear warnings about it's lower performance and run-time / build-time, per render / per component use / per component definition tradeoffs, and the recommended transformer approach. Once a project starts to feel pains from rendering significant amounts of vector graphics on low end resource constrained devices, there exists a path to get some improvements with minimal configuration changes. |
@Riseley Could you test the LocalSvg component? Would that api cover your use case? |
This could probably be released by now, together with patch-package and those three patches it's possible to import and/or require svg files, and use them with LocalSvg or with Animated and WithLocalSvg, and it should all work on both ios and android, debug and release builds. |
There's a second commit implementing WithLocalSvg in the develop branch atm: e66e87a |
But yeah, I think it's about time for me to take a step back from open-source development. It's starting to feel strangely tiring and I don't really have the time to spare. Certainly about time that more people in the community step up to do their part of the work involved in this repository. |
@msand, hi, sorry for the delay, the WithLocalSvg in the develop branch atm: e66e87a perfectly covered my use case. It would help many new users and end this issue once in for all. So it will be cool if you can add it. Or you can also not add it to make the library as small as possible. It is your decision. Maintaining open source is a underappreciated one. But for those who do appreciate, we are grateful that you even created this repo, and the Github Stars clear show that what you are doing is important and widely appreciated. In the end, after many years most of us realize that it is not what we get from the world that is important, but what we put into the world. I don't have a good open source project that generate so much stars because I am still inexperienced, and I can't really read long codes and do PRs for you like a pro, I am still learning. But you have one great and popular library, that is extremely useful to many, I think you staying would inspire many others, at least you are inspiring me. But of course, I understand it is tiring, but Github Sponsors help. Please try out https://github.com/sponsors. Some gratitude given now and then is satisfying in the long run. |
# [12.1.0](v12.0.3...v12.1.0) (2020-04-09) ### Bug Fixes * **web:** improve react-native-web version compatibility ([88953c3](88953c3)) ### Features * implement WithLocalSvg ([e66e87a](e66e87a)) * Support local .svg files, fixes [#1306](#1306) ([4e9e8b5](4e9e8b5)) * **svgUri:** add onError prop to SvgUri/Xml/Ast ([3c32a6f](3c32a6f))
🎉 This issue has been resolved in version 12.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Hello Guys, First of all thanks for your valuable work! Not sure if it is the right place to ask but as I found this thread I decided to post here. I been trying to use this new component Trying it on IOS. My code:
I have tried to use Thanks a lot. |
@diazevedo Hope you have already solved the issue, import { WithLocalSvg} from 'react-native-svg'; |
@diazevedo were you able to resolve it ? |
Just remove react-native-svg-transformer's settings from metro.config.js |
If anyone comes across the thread this is how I ultimately ended up solving this issue in the expo 42, react-native: 42. Be sure to take away react-native-svg-transformer if you have put any of that in your project.
|
Maybe something has changed or just a misspell but
notice the object destructuring. And it actually works (without |
Thank you for your time and energy for maintaining this open source project.
I expected easy local file support like
react-native-svg-uri
so I spent quite some time to search for similar issue in this repository but surprisingly couldn't find any. Hence opening this issue.Description of Bug & Unexpected behavior, and Steps To Reproduce
Mimicking the README example, I do
Remote urls like the original
uri="http://thenewcode.com/assets/images/thumbnails/homer-simpson.svg"
works perfectly, but is blank (not rendering anything) for local files.This worked in
react-native-svg-uri
but in that thread you also stated thatreact-native-svg
now supports this functionality. And considering that people are still defaulting to usingreact-native-svg-uri
to this day it means that this buggy and unexpected behavior is losing customers as they are using the workaround. It is okay if the workaround is good, the problem is the workaround,react-native-svg-uri
, is buggy and no longer active and sincereact-native-svg
supports this functionality, I assume that you would want to look into this issue to help out everyone who initially tried to use this library but faced similar problems.If you do not correct the default behavior, then similar issues would be opened again and again the issues always end with the workaround's creator asking people to use the workaround, e.g. #1204, #310, #612, #510, #347, #310, expo/expo#2402, which is not ideal and keeps causing confusions, as people will always ask you something about the workaround, which I am sure you are already fed up with.
Environment info
The text was updated successfully, but these errors were encountered: