aidenybai / million

Optimize React performance and make your React 70% faster in minutes, not months.
https://million.dev
MIT License
15.89k stars 558 forks source link

Automatic compiler renders SVG with invalid props #996

Closed AlexanderArvidsson closed 1 month ago

AlexanderArvidsson commented 3 months ago

What version of million are you using?

3.0.6

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

yarn

What operating system are you using?

Linux

What browser are you using?

Firefox

Describe the Bug

The automatic compiler is turning my icon props into invalid HTML props. See example:

import React from 'react'
import IconProps from './IconProps'

export interface IconCheckCircle2Props extends IconProps {
  className?: string
}

const IconCheckCircle2: React.FC<React.PropsWithChildren<IconCheckCircle2Props>> = ({
  className,
  color = 'currentColor',
  ...props
}) => {
  return (
    <svg
      {...{ className }}
      width={props.size ?? props.width ?? '1em'}
      height={props.size ?? props.height ?? '1em'}
      viewBox="0 0 24 24"
      fill="none"
      stroke={color}
      strokeWidth="2"
      strokeLinecap="round"
      strokeLinejoin="round"
      xmlns="http://www.w3.org/2000/svg">
      <path d="M12 22c5.523 0 10-4.477 10-10S17.523 2 12 2 2 6.477 2 12s4.477 10 10 10z"></path>
      <path d="M9 12l2 2 4-4"></path>
    </svg>
  )
}

export default IconCheckCircle2

This is rendered with invalid properties for strokeWidth, etc, as they show up as strokewidth instead of stroke-width:

<g>
   <svg $="v0" viewBox="0 0 24 24" fill="none" strokewidth="2" strokelinecap="round" strokelinejoin="round" xmlns="http://www.w3.org/2000/svg" width="1.25em" height="1.25em" stroke="currentColor">
      <path d="M12 22c5.523 0 10-4.477 10-10S17.523 2 12 2 2 6.477 2 12s4.477 10 10 10z"></path>
      <path d="M9 12l2 2 4-4"></path>
   </svg>
</g>

In the StackBlitz, try exporting App over AppBlock to see the difference (and remove the AppBlock definition to prevent patching).

Also note the random <g> element wrapping the SVG, which does not exist in the output of the non-million code.

I couldn't get the <g> element to show up in the reproduction StackBlitz though.

What's the expected result?

It should be rendered with valid HTML properties, as is the case when we're not using Million:

<svg width="1.25em" height="1.25em" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" xmlns="http://www.w3.org/2000/svg">
   <path d="M12 22c5.523 0 10-4.477 10-10S17.523 2 12 2 2 6.477 2 12s4.477 10 10 10z"></path>
   <path d="M9 12l2 2 4-4"></path>
</svg>

Also, there is no random <g> wrapper element.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-krvb8s?file=src%2FIcon.jsx

Participation

github-actions[bot] commented 3 months ago

Thanks for opening this issue! A maintainer will review it soon.

AlexanderArvidsson commented 3 months ago

I did some digging and the react-dom includes a list of all aliases:

const aliases = new Map([
  ['acceptCharset', 'accept-charset'],
  ['htmlFor', 'for'],
  ['httpEquiv', 'http-equiv'],
  // HTML and SVG attributes, but the SVG attribute is case sensitive.],
  ['crossOrigin', 'crossorigin'],
  // This is a list of all SVG attributes that need special casing.
  // Regular attributes that just accept strings.],
  ['accentHeight', 'accent-height'],
  ['alignmentBaseline', 'alignment-baseline'],
  ['arabicForm', 'arabic-form'],
  ['baselineShift', 'baseline-shift'],
  ['capHeight', 'cap-height'],
  ['clipPath', 'clip-path'],
  ['clipRule', 'clip-rule'],
  ['colorInterpolation', 'color-interpolation'],
  ['colorInterpolationFilters', 'color-interpolation-filters'],
  ['colorProfile', 'color-profile'],
  ['colorRendering', 'color-rendering'],
  ['dominantBaseline', 'dominant-baseline'],
  ['enableBackground', 'enable-background'],
  ['fillOpacity', 'fill-opacity'],
  ['fillRule', 'fill-rule'],
  ['floodColor', 'flood-color'],
  ['floodOpacity', 'flood-opacity'],
  ['fontFamily', 'font-family'],
  ['fontSize', 'font-size'],
  ['fontSizeAdjust', 'font-size-adjust'],
  ['fontStretch', 'font-stretch'],
  ['fontStyle', 'font-style'],
  ['fontVariant', 'font-variant'],
  ['fontWeight', 'font-weight'],
  ['glyphName', 'glyph-name'],
  ['glyphOrientationHorizontal', 'glyph-orientation-horizontal'],
  ['glyphOrientationVertical', 'glyph-orientation-vertical'],
  ['horizAdvX', 'horiz-adv-x'],
  ['horizOriginX', 'horiz-origin-x'],
  ['imageRendering', 'image-rendering'],
  ['letterSpacing', 'letter-spacing'],
  ['lightingColor', 'lighting-color'],
  ['markerEnd', 'marker-end'],
  ['markerMid', 'marker-mid'],
  ['markerStart', 'marker-start'],
  ['overlinePosition', 'overline-position'],
  ['overlineThickness', 'overline-thickness'],
  ['paintOrder', 'paint-order'],
  ['panose-1', 'panose-1'],
  ['pointerEvents', 'pointer-events'],
  ['renderingIntent', 'rendering-intent'],
  ['shapeRendering', 'shape-rendering'],
  ['stopColor', 'stop-color'],
  ['stopOpacity', 'stop-opacity'],
  ['strikethroughPosition', 'strikethrough-position'],
  ['strikethroughThickness', 'strikethrough-thickness'],
  ['strokeDasharray', 'stroke-dasharray'],
  ['strokeDashoffset', 'stroke-dashoffset'],
  ['strokeLinecap', 'stroke-linecap'],
  ['strokeLinejoin', 'stroke-linejoin'],
  ['strokeMiterlimit', 'stroke-miterlimit'],
  ['strokeOpacity', 'stroke-opacity'],
  ['strokeWidth', 'stroke-width'],
  ['textAnchor', 'text-anchor'],
  ['textDecoration', 'text-decoration'],
  ['textRendering', 'text-rendering'],
  ['transformOrigin', 'transform-origin'],
  ['underlinePosition', 'underline-position'],
  ['underlineThickness', 'underline-thickness'],
  ['unicodeBidi', 'unicode-bidi'],
  ['unicodeRange', 'unicode-range'],
  ['unitsPerEm', 'units-per-em'],
  ['vAlphabetic', 'v-alphabetic'],
  ['vHanging', 'v-hanging'],
  ['vIdeographic', 'v-ideographic'],
  ['vMathematical', 'v-mathematical'],
  ['vectorEffect', 'vector-effect'],
  ['vertAdvY', 'vert-adv-y'],
  ['vertOriginX', 'vert-origin-x'],
  ['vertOriginY', 'vert-origin-y'],
  ['wordSpacing', 'word-spacing'],
  ['writingMode', 'writing-mode'],
  ['xmlnsXlink', 'xmlns:xlink'],
  ['xHeight', 'x-height'],
]);

This issue is probably relevant for the above non-SVG properties as well.

In Million template.ts, these lines should be expanded to include all the above properties:

    if (name === 'className') name = 'class';
    if (name === 'htmlFor') name = 'for';
tobySolutions commented 3 months ago

Hey @AlexanderArvidsson, thanks for this. Tagging @lxsmnsyc, @NisargIO to this.

AlexanderArvidsson commented 3 months ago

I've got a PR on its way with full alias list and associated test, just validating if it works in our application first. Edit: Tested in our app and works! Also works in your demo kitchen sink. PR is up: #997

This fix does not account for the random <g>, and I suspect that is a separate issue that I have not looked into at the moment. Since I can't reproduce it in StackBlitz, I'll get back to you if I find a reproducible way for this. Seems related to #957 though

github-actions[bot] commented 3 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs within the next 7 days.

AlexanderArvidsson commented 3 months ago

Not stale, PR is up: https://github.com/aidenybai/million/pull/997

github-actions[bot] commented 2 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs within the next 7 days.

AlexanderArvidsson commented 2 months ago

Still not stale, PR is up: https://github.com/aidenybai/million/pull/997

tobySolutions commented 2 months ago

Can you please take a look at this @lxsmnsyc, @aidenybai? Thank you.