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

SvgXml implementation for web #1431

Closed

Conversation

MatheusrdSantos
Copy link

Summary

Explain the motivation for making this change: here are some points to help you:

  • What issues does the pull request solve? Please tag them so that they will get automatically closed once the PR is merged
    'SvgXml' for web #1279
    Attempted import error: 'SvgXml' is not exported from 'react-native-svg'. #1236

  • What is the feature? (if applicable)
    This PR contains the implementation of SvgXml for web. This web implementation has exactly the same usage as the native implementation.

  • How did you implement the solution?
    The web implementation relies on unstable_createElement and View from react-native-web. So, basically I used unstable_createElement to create an <svg> element with the props that comes from the XML string and set the innerHTML from the <svg> to be the XML content.
    In order to accept react-native props (the same as SvgXml native implementation), I used the <View> to wrap the <svg> and simulate the same behavior.

  • What areas of the library does it impact?
    None. It just brings a new supported component for web.

Test Plan

You can check a working demo here: https://codesandbox.io/s/keen-platform-z40rg

Compatibility

OS Implemented
iOS
Android
Web

Checklist

  • I have tested this on browser

@kofsiwon
Copy link

I hope release this quickly. I need SvgXml for react native web!!

@Sharcoux
Copy link

For those still needing this, you can try our package: @cantoo/rn-svg

@MattFoley
Copy link

@WoLewicki @MatheusrdSantos @Sharcoux Should we merge this in? It seems risky long term to have a fork maintain this functionality. Thanks!

@Sharcoux
Copy link

@MattFoley I'm glad that the project revived.

We can rebase our fork and make it ready for merge, that would be awesome. However, there is a concern you should know about:

We had to wrap the svg inside a View. This has impacts when trying to style the SVG, especially with everything related to layout. We made a system that distribute the styles between the wrapper and the svg itself, but it will very likely introduce problems in some layouts. unstable_createElement would not let us pass down every react-native props to the underlying element. Another valid approach could be to only export the element created with unstable_createElement and give up on unsupported props.

You'll find all the details here: necolas/react-native-web#1686

@MattFoley
Copy link

Hmm, I'll be honest I don't fully understand the debate on RNW, and would need more time to dive into the specifics. I also am using v14.1 of rn-svg, and don't think I can downgrade to 13. I started to rebase your fork up to v14.1 myself, but haven't finished yet, working through some BoB type errors.

@Sharcoux
Copy link

Ouch, I missed your comment, sorry.

No, don't rebase this here. If you want to follow our approach, we need to push our current version of it. Many fixes have been made since this.

@Sharcoux
Copy link

Moreover, @MattFoley there should not be any rebase needed because we only need to add new files. Not edit existing ones.

@MattFoley
Copy link

@Sharcoux I would love if you had a new version you could push up on 14.x!

@bohdanprog
Copy link
Member

Hello,
@MatheusrdSantos, could you please explain to me what the difference is between that solution and our current implementation?

Thank you.

@MatheusrdSantos MatheusrdSantos changed the base branch from develop to main July 8, 2024 19:19
@MatheusrdSantos
Copy link
Author

Hi, @bohdanprog

this PR is just adding the web implementation for the SvgXml component, which is currently missing from the main package.
It uses react-native-web to keep the react-native syntax, just as the current web implementation for Svg, G, Path... does.

The main difference is the need to import some hooks that are not exposed by the react-native-web api. Those hooks are important to emulate the native behavior, and therefore, make it possible to watch layout changes, measure the element and listen for touches by using the same native api.

This PR (#1686) has the same implementation I did, but it's not rebased. Anyway it's a good complement to my explanation.

@Sharcoux Sharcoux mentioned this pull request Jul 12, 2024
3 tasks
@bohdanprog bohdanprog self-assigned this Jul 25, 2024
@bohdanprog bohdanprog requested a review from jakex7 July 26, 2024 09:23
@bohdanprog
Copy link
Member

Hi @MatheusrdSantos,
I’ve improved some parts of your code and hope you don’t mind.
Could you take a look at my changes and let me know what you think?
Thank you!

@MatheusrdSantos
Copy link
Author

MatheusrdSantos commented Jul 29, 2024

@bohdanprog The changes are good, thanks for the code improvements.

There's just one change I didn't understand. Is there a reason to move the react-native-web package from peerDependencies/optionalDependencies to dependencies?

I think we could keep this dependency as optional because not all user may want to use the web implementation.

@bohdanprog
Copy link
Member

Hi @MatheusrdSantos,

We have made some updates to react-native-svg, and as far as I know, we can now display SvgXml, SvgCss, etc.
Would you like to verify this?
Thank you.

@Sharcoux
Copy link

Sharcoux commented Aug 5, 2024

I tried. SvgXml is undefined if I import import { SvgXml } from 'react-native-web'. When I check the code, I see that there is an implementation in the file src/xml.tsx. However, I don't understand how that implementation is supposed to be made available for the web, as it is never exported from ReactNativeSVG.web.ts

I think that you want to copy the lines 29 to 47 from ReactNativeSVG.ts into ReactNativeSVG.web.ts:

import {
  parse,
  SvgAst,
  SvgFromUri,
  SvgFromXml,
  SvgUri,
  SvgXml,
  camelCase,
  fetchText,
  JsxAST,
  Middleware,
  Styles,
  UriProps,
  UriState,
  XmlAST,
  XmlProps,
  XmlState,
  AstProps,
} from './xml';

But also the lines 141 to 146 and 178 to 188:

image

export type {
  JsxAST,
  Middleware,
  Styles,
  UriProps,
  UriState,
  XmlAST,
  XmlProps,
  XmlState,
  AstProps,
};

@bohdanprog
Copy link
Member

Hi @Sharcoux,
Can you please try to run the test app from the main branch?
Those functionalities are already available if you use the library from the main branch.
I believe we are going to create the new release of react-native-svg today.

@bohdanprog
Copy link
Member

@Sharcoux FYI, now it should work when you are using react-native-svg 15.5.0

@jakex7
Copy link
Member

jakex7 commented Aug 12, 2024

Closing as it has been implemented in https://github.com/software-mansion/react-native-svg/releases/tag/v15.5.0

@jakex7 jakex7 closed this Aug 12, 2024
@MatheusrdSantos
Copy link
Author

MatheusrdSantos commented Aug 19, 2024

Hi, @bohdanprog
I tested the version 15.5.0. There are two issues.

The first error is happening because the dependency @react-native/assets-registry is missing in the package.json file from react-native-svg. It can be easily fixed by manually installing the dependency.

Module not found: Can't resolve '@react-native/assets-registry/registry' in '/home/matheus/cantoo/cantoo-monorepo/node_modules/react-native-svg/lib/module/lib'
node_modules/react-native-svg/lib/module/lib/resolveAssetUri.js

The second issue is that @react-native/assets-registry uses flow to check its types and doesn't expose a built version for general js usage.
There are two open discussions about this issue:
fix: Remove Flow from @react-native/assets-registry - registry.js: facebook/react-native#44002
Build assets-registry for general consumption: facebook/react-native#39440

Module parse failed: Unexpected token (13:7)
node_modules/@react-native/assets-registry/registry.js
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
| 'use strict';
| 
> export type PackagerAsset = {
|   +__packager_asset: boolean,
|   +fileSystemLocation: string,

@bohdanprog
Copy link
Member

@MatheusrdSantos Can you tell me please, how can I reproduce that issue?

@MatheusrdSantos
Copy link
Author

@bohdanprog I built a simple project to reproduce the issue. It's just importing the react-native-svg lib.

I'm using webpack because I need react-native-web to use the web implementation of react-native-svg.

yarn add @babel/core @babel/preset-env @babel/preset-react babel-loader html-webpack-plugin webpack webpack-cli react-native-svg@15.5.0 react-native-web@0.19.8 -D

src/App.js

import React from "react";
import { SvgXml } from "react-native-svg";
const App = () => {
  return <h1>Title</h1>;
};

export default App;

webpack.config.js

const path = require("path");
const HtmlWebpackPlugin = require("html-webpack-plugin");

module.exports = {
  output: {
    path: path.join(__dirname, "/dist"),
    filename: "bundle.js",
  },
  plugins: [
    new HtmlWebpackPlugin({
      template: "src/index.html",
    }),
  ],
  module: {
    rules: [
      {
        test: /\.(js|jsx)$/,
        exclude: /node_modules/,
        use: {
          loader: "babel-loader",
        },
      },
    ],
  },
  resolve: {
    extensions: ['.web.ts', '.ts', '.web.js', '.js'],
    alias: {
      'react-native$': 'react-native-web'
    }
  }
};

src/index.js

import React from "react";
import ReactDOM from "react-dom";
import App from "./App";

const el = document.getElementById("app");

ReactDOM.render(<App />, el);

src/index.html

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta http-equiv="X-UA-Compatible" content="IE=edge" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title></title>
  </head>
  <body>
    <div id="app"></div>
    <script src="bundle.js"></script>
  </body>
</html>

.babelrc

{
  "presets": ["@babel/preset-env", "@babel/preset-react"]
}

If I try to run this project npm run webpack --mode production, the build fails because the dependency @react-native/assets-registry/registry is missing.

ERROR in ./node_modules/react-native-svg/lib/module/lib/resolveAssetUri.js 3:0-70
Module not found: Error: Can't resolve '@react-native/assets-registry/registry' in '/Users/matheus/cantoo/rn-svg-sample/node_modules/react-native-svg/lib/module/lib'

If I manually add the dependency yarn add @react-native/assets-registry, I get an issue related to webpack not being able to bundle the file ./node_modules/@react-native/assets-registry/registry.js.
I think this second issue isn't directly related to react-native-svg, but it impacts the end users.

ERROR in ./node_modules/@react-native/assets-registry/registry.js 13:7
Module parse failed: Unexpected token (13:7)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
| 'use strict';
| 
> export type PackagerAsset = {
|   +__packager_asset: boolean,
|   +fileSystemLocation: string,

@bohdanprog
Copy link
Member

Hi @MatheusrdSantos, IDK why but your simple repro isn't simple :D
I created a repository with a simple example, can you modify that?

@MatheusrdSantos
Copy link
Author

MatheusrdSantos commented Aug 20, 2024

@bohdanprog I created this pull request: bohdanprog/RNSVG-XML-web#1.
I noticed that @react-native/assets-registry/registry was missing because react-native-svg@15.5.0 requires the react-native version to be ^0.72.3. I was using an older version, so it was my falt.

But the second issue remains.

@bohdanprog
Copy link
Member

bohdanprog commented Aug 20, 2024

@MatheusrdSantos Hm as I see that issue exists when we use the webpack configuration, but IDK why.
Maybe is it a matter of configuration?
I guess it's the same issue

@MatheusrdSantos
Copy link
Author

MatheusrdSantos commented Aug 20, 2024

@bohdanprog yes, it's the same issue.

The webpack issue is a matter of configuration that will impact all react-native-svg users that don't have a proper config to parse flow files. So it's a breaking change for those users.

@bohdanprog
Copy link
Member

@MatheusrdSantos I added the correct webpack config to the example app, can you check that?

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.

6 participants