Replace rehype with svg-parser #321

TrySound · 2019-07-05T13:03:19Z

Context:

https://npm.anvaka.com/#/view/2d/rehype
https://packagephobia.now.sh/result?p=rehype
https://packagephobia.now.sh/result?p=svg-parser
https://npm.anvaka.com/#/view/2d/svg-parser

Did not notice any changes in speed. Though startup time is probably
better.

https://npm.anvaka.com/#/view/2d/rehype https://packagephobia.now.sh/result?p=rehype https://packagephobia.now.sh/result?p=svg-parser https://npm.anvaka.com/#/view/2d/svg-parser Did not notice any changes in speed. Though startup time is probably better.

vercel · 2019-07-05T13:03:25Z

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

TrySound · 2019-07-05T13:10:17Z

Another point with reasonml can be solved with templating a simple string instead of babel stuff. I want to send a PR too.

codecov · 2019-07-05T13:11:56Z

Codecov Report

Merging #321 into master will decrease coverage by 0.36%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #321      +/-   ##
=========================================
- Coverage   86.66%   86.3%   -0.37%     
=========================================
  Files          30      27       -3     
  Lines         480     460      -20     
  Branches      134     129       -5     
=========================================
- Hits          416     397      -19     
+ Misses         53      52       -1     
  Partials       11      11

Impacted Files	Coverage Δ
packages/hast-util-to-babel-ast/src/index.js	`100% <100%> (ø)`	⬆️
packages/plugin-jsx/src/index.js	`100% <100%> (ø)`	⬆️
...ckages/hast-util-to-babel-ast/src/getAttributes.js	`90% <100%> (-3.34%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06e1e99...78a835c. Read the comment docs.

codecov · 2019-07-05T13:11:56Z

Codecov Report

Merging #321 into master will decrease coverage by 0.36%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #321      +/-   ##
=========================================
- Coverage   86.66%   86.3%   -0.37%     
=========================================
  Files          30      27       -3     
  Lines         480     460      -20     
  Branches      134     129       -5     
=========================================
- Hits          416     397      -19     
+ Misses         53      52       -1     
  Partials       11      11

Impacted Files	Coverage Δ
packages/hast-util-to-babel-ast/src/getProps.js	`90% <100%> (ø)`
packages/plugin-jsx/src/index.js	`100% <100%> (ø)`	⬆️
packages/hast-util-to-babel-ast/src/index.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06e1e99...9ae1c1b. Read the comment docs.

gregberge · 2019-07-08T06:55:11Z

Hello @TrySound, the whole point of SVGR is to be robust, I think moving to a un-maintained SVG parser is not good idea. Especially if the only gain is 600ko. Also they are chance that rehype would be used by other packages in the project (all markdown ecosystem) so it could result in a bigger node_modules directory at the end.

In fact I could be OK but I am sure we will have issue with some weird SVG not parsed correctly.

TrySound · 2019-07-08T07:16:41Z

svg-parser is not really unmaintained. It's complete project and no bugs are found yet :) Rich released my PR in a couple of hours 3 days ago.

Rehype can be so complex because of parsing html syntax which is very loose. SVG is strict XML, no need to cover all html use cases.

I ran new code on 150 icons in my project and they compiled without problems.

gregberge · 2019-07-08T07:24:19Z

OK Thanks, so maybe we should consider it.

TrySound · 2019-07-08T12:42:01Z

@neoziro Rich is ok to migrate to hast so we do not need to make changes in hast-util-to-babel-ast.

TrySound · 2019-07-08T20:04:23Z

Rich-Harris/svg-parser#3

codecov-io · 2019-07-09T13:10:45Z

Codecov Report

Merging #321 into master will decrease coverage by 1.66%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #321      +/-   ##
========================================
- Coverage   86.66%    85%   -1.67%     
========================================
  Files          30     30              
  Lines         480    480              
  Branches      134    134              
========================================
- Hits          416    408       -8     
- Misses         53     60       +7     
- Partials       11     12       +1

Impacted Files	Coverage Δ
packages/plugin-jsx/src/index.js	`100% <100%> (ø)`	⬆️
packages/hast-util-to-babel-ast/src/handlers.js	`69.56% <0%> (-30.44%)`	⬇️
...ckages/hast-util-to-babel-ast/src/getAttributes.js	`90% <0%> (-3.34%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06e1e99...cea6947. Read the comment docs.

TrySound · 2019-07-09T13:21:43Z

@neoziro I reverted all changes in hast-util-to-babel-ast so we don't have to rename the plugin :)

wooorm · 2019-07-11T07:35:47Z

Hi folks! I don’t want to pick a fight, if you want to switch to something that’s fine, but I would be cautious about it!

rehype-parse uses parse5, which is often already in node_modules anyway (through jsdom). unified and vfile are typically in node modules as well (such as through prettier).

svg-parser is not really unmaintained. It's complete project and no bugs are found yet :) Rich released my PR in a couple of hours 3 days ago.

It may be complete, or it could be that it isn’t used a lot (2.5k downloads a week) compared to rehype-parse (1.2m a week), so bugs aren't found.

Rich-Harris/svg-parser#3

Oh that’s cool! Be careful though, hast properties aren’t more complex than settings attribute names and values on an object: https://github.com/syntax-tree/hast#properties

One reason to switch to something else from rehype-parse is that rehype-parse is for HTML, and it supports SVG in HTML. But it’s not made just for SVG.

TrySound · 2019-07-11T07:45:36Z

Hi! Thanks for answer.

Not really true about prettier. It bundles all dependencies.
https://unpkg.com/[email protected]/package.json

Properties in svg-parser are either string or numbers. There should not be problems.

wooorm · 2019-07-11T08:03:52Z

Not really true about prettier. It bundles all dependencies.

Ah, interesting! Well prettier was one example, but in that case the usage is even higher than I thought ;)

Properties in svg-parser are either string or numbers. There should not be problems.

What I mean is that the casing of hast properties is different, and that values are not always strings or numbers. It may work, but it’s not conforming hast, so it also may not work.

slorber · 2019-07-15T12:54:22Z

Hi,

The new release gives me an output that react-native-svg won't understand. It worked in friday:

Original SVG:

<?xml version="1.0" encoding="UTF-8"?>
<svg width="30px" height="30px" viewBox="0 0 30 30" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
    <!-- Generator: Sketch 55.2 (78181) - https://sketchapp.com -->
    <title>Icons/PièceNuit/30px/Black</title>
    <desc>Created with Sketch.</desc>
    <g id="Icons/PièceNuit/30px/Black" stroke="none" stroke-width="1" fill="none" fill-rule="evenodd">
        <path d="M24.749132,24.0262157 C24.3556455,23.9469222 23.962159,23.8501007 23.5820565,23.7382551 C19.6355926,22.5805699 16.584065,19.8286681 15.2099853,16.1878443 C13.8359056,12.5470206 14.363231,8.6132282 16.6581225,5.39391201 C16.8802951,5.08258069 17.1203129,4.77709203 17.3728224,4.48495808 C17.450449,4.39481389 17.4656174,4.27128296 17.4120818,4.16778407 C17.357654,4.06428518 17.2452293,3.99918104 17.1220975,4.00001571 C15.7890617,4.01670908 14.4703021,4.23706155 13.2006168,4.65689978 C6.80936175,6.76777627 3.4455433,13.349137 5.70206766,19.327867 C7.95859203,25.3074318 14.9949507,28.4541318 21.3862058,26.3432553 C22.6558911,25.9242517 23.8301046,25.3207865 24.8785096,24.5503875 C24.9757659,24.4794407 25.0194866,24.3625871 24.9918266,24.2499068 C24.9641665,24.1372266 24.869587,24.0504211 24.749132,24.0262157 Z" id="Fill-1" stroke="#20222B" stroke-width="2.1"></path>
    </g>
</svg>

This works (manual rework)

import React from 'react';
import Svg, { G, Path, Circle, SvgProps } from 'react-native-svg';

const SVGMoon = (props: SvgProps) => (
  <Svg width="1em" height="1em" viewBox="0 0 30 30" {...props}>
    <Path
      d="M24.749132,24.0262157 C24.3556455,23.9469222 23.962159,23.8501007 23.5820565,23.7382551 C19.6355926,22.5805699 16.584065,19.8286681 15.2099853,16.1878443 C13.8359056,12.5470206 14.363231,8.6132282 16.6581225,5.39391201 C16.8802951,5.08258069 17.1203129,4.77709203 17.3728224,4.48495808 C17.450449,4.39481389 17.4656174,4.27128296 17.4120818,4.16778407 C17.357654,4.06428518 17.2452293,3.99918104 17.1220975,4.00001571 C15.7890617,4.01670908 14.4703021,4.23706155 13.2006168,4.65689978 C6.80936175,6.76777627 3.4455433,13.349137 5.70206766,19.327867 C7.95859203,25.3074318 14.9949507,28.4541318 21.3862058,26.3432553 C22.6558911,25.9242517 23.8301046,25.3207865 24.8785096,24.5503875 C24.9757659,24.4794407 25.0194866,24.3625871 24.9918266,24.2499068 C24.9641665,24.1372266 24.869587,24.0504211 24.749132,24.0262157 Z"
      stroke="currentColor"
      strokeWidth={2.1}
      fill="none"
      fillRule="evenodd"
    />
  </Svg>
);

export default SVGMoon;

This is direct output from new release, and won't work due to bad path:

import React from 'react'
import Svg, { Path } from 'react-native-svg'
/* SVGR has dropped some elements not supported by react-native-svg: title */

const SvgComponent = props => (
  <Svg width="1em" height="1em" viewBox="0 0 30 30" {...props}>
    <Path
      d="M24.75 24.026c-.394-.08-.788-.176-1.168-.288-3.946-1.157-6.998-3.91-8.372-7.55-1.374-3.641-.847-7.575 1.448-10.794.222-.311.462-.617.715-.909a.283.283 0 00.04-.317.323.323 0 00-.29-.168 12.99 12.99 0 00-3.922.657c-6.392 2.11-9.755 8.692-7.499 14.67 2.257 5.98 9.293 9.127 15.684 7.016a12.495 12.495 0 003.493-1.793.286.286 0 00.113-.3.308.308 0 00-.243-.224z"
      stroke="#20222B"
      strokeWidth={2.1}
      fill="none"
      fillRule="evenodd"
    />
  </Svg>
)

export default SvgComponent

Edit: I don't know if this is due to the new parser or not. Maybe it's just SVGO need to be removed for RN as without it, it works. Don't remember having unchecked SVGO last week so maybe a default setting has changed?

gregberge · 2019-07-15T15:31:00Z

Try before the latest version, if it works it is the parser and I will probably revert it if we have other issues.

TrySound · 2019-07-15T15:44:48Z

I got the same with 4.3.1. Looks like svgo issue.

Replace rehype with svg-parser

c102c90

https://npm.anvaka.com/#/view/2d/rehype https://packagephobia.now.sh/result?p=rehype https://packagephobia.now.sh/result?p=svg-parser https://npm.anvaka.com/#/view/2d/svg-parser Did not notice any changes in speed. Though startup time is probably better.

vercel bot deployed to staging July 5, 2019 13:03 View deployment

Fix lint

78a835c

vercel bot deployed to staging July 5, 2019 13:09 View deployment

Rename getAttributes to getProps

449ea7f

vercel bot deployed to staging July 5, 2019 13:14 View deployment

Upgrade to svg-parser without dependencies

9ae1c1b

vercel bot deployed to staging July 5, 2019 13:36 View deployment

Upgrade svg-parser with hast support

4669920

vercel bot deployed to staging July 9, 2019 13:10 View deployment

Remove unified

5779618

vercel bot requested a deployment to staging July 9, 2019 13:14 Pending

vercel bot temporarily deployed to staging July 9, 2019 13:15 Inactive

Remove getProps.js

cea6947

vercel bot deployed to staging July 9, 2019 13:16 View deployment

gregberge merged commit 7eb5ef6 into gregberge:master Jul 15, 2019

TrySound deleted the svg-parser branch July 15, 2019 08:53

TrySound mentioned this pull request Jul 15, 2019

Crash in last version svgo 1.3.0 #327

Closed

sudkumar mentioned this pull request Sep 25, 2019

Attribute renamed after svgr #323

Closed

wooorm mentioned this pull request Sep 29, 2020

docs: add hast-util-to-babel-ast syntax-tree/hast#21

Closed

jonkiky mentioned this pull request Oct 1, 2024

[Snyk] Upgrade @svgr/webpack from 4.1.0 to 4.3.3 CBIIT/INS-WebPortal#345

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace rehype with svg-parser #321

Replace rehype with svg-parser #321

TrySound commented Jul 5, 2019

vercel bot commented Jul 5, 2019 •

edited

Loading

TrySound commented Jul 5, 2019

codecov bot commented Jul 5, 2019

codecov bot commented Jul 5, 2019 •

edited

Loading

gregberge commented Jul 8, 2019

TrySound commented Jul 8, 2019

gregberge commented Jul 8, 2019

TrySound commented Jul 8, 2019

TrySound commented Jul 8, 2019

codecov-io commented Jul 9, 2019 •

edited by codecov bot

Loading

TrySound commented Jul 9, 2019

wooorm commented Jul 11, 2019

TrySound commented Jul 11, 2019

wooorm commented Jul 11, 2019

slorber commented Jul 15, 2019 •

edited

Loading

gregberge commented Jul 15, 2019

TrySound commented Jul 15, 2019

Replace rehype with svg-parser #321

Replace rehype with svg-parser #321

Conversation

TrySound commented Jul 5, 2019

vercel bot commented Jul 5, 2019 • edited Loading

TrySound commented Jul 5, 2019

codecov bot commented Jul 5, 2019

Codecov Report

codecov bot commented Jul 5, 2019 • edited Loading

Codecov Report

gregberge commented Jul 8, 2019

TrySound commented Jul 8, 2019

gregberge commented Jul 8, 2019

TrySound commented Jul 8, 2019

TrySound commented Jul 8, 2019

codecov-io commented Jul 9, 2019 • edited by codecov bot Loading

Codecov Report

TrySound commented Jul 9, 2019

wooorm commented Jul 11, 2019

TrySound commented Jul 11, 2019

wooorm commented Jul 11, 2019

slorber commented Jul 15, 2019 • edited Loading

gregberge commented Jul 15, 2019

TrySound commented Jul 15, 2019

vercel bot commented Jul 5, 2019 •

edited

Loading

codecov bot commented Jul 5, 2019 •

edited

Loading

codecov-io commented Jul 9, 2019 •

edited by codecov bot

Loading

slorber commented Jul 15, 2019 •

edited

Loading