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

Automatic compiler renders SVG with invalid props #996

Closed
1 task done
AlexanderArvidsson opened this issue Mar 17, 2024 · 9 comments · Fixed by #997 or WomB0ComB0/gdsc-farmingdale-links-api#16
Closed
1 task done

Comments

@AlexanderArvidsson
Copy link
Contributor

AlexanderArvidsson commented Mar 17, 2024

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

  • I am willing to submit a pull request for this issue.
Copy link

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

@AlexanderArvidsson AlexanderArvidsson changed the title 56 Automatic compiler renders SVG with invalid props Mar 17, 2024
@AlexanderArvidsson
Copy link
Contributor Author

AlexanderArvidsson commented Mar 17, 2024

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
Copy link
Contributor

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

@AlexanderArvidsson
Copy link
Contributor Author

AlexanderArvidsson commented Mar 17, 2024

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

AlexanderArvidsson added a commit to AlexanderArvidsson/million that referenced this issue Mar 17, 2024
Copy link

github-actions bot commented Apr 2, 2024

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.

@github-actions github-actions bot added the Stale label Apr 2, 2024
@AlexanderArvidsson
Copy link
Contributor Author

Not stale, PR is up: #997

@github-actions github-actions bot removed the Stale label Apr 3, 2024
Copy link

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.

@github-actions github-actions bot added the Stale label Apr 18, 2024
@AlexanderArvidsson
Copy link
Contributor Author

Still not stale, PR is up: #997

@github-actions github-actions bot removed the Stale label Apr 20, 2024
@tobySolutions
Copy link
Contributor

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

lxsmnsyc pushed a commit that referenced this issue May 9, 2024
* fix: #996 Handle property alias

* fix: Lint - Unnamed function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants