octicons icon indicating copy to clipboard operation
octicons copied to clipboard

feat(octicons_react): replace usage of `dangerouslySetInnerHTML`

Open joshblack opened this issue 3 years ago • 3 comments

This is a follow-up to the experiment in: https://github.com/primer/octicons/pull/837

Closes https://github.com/primer/octicons/issues/796

What's changed

At a high-level, this PR changes a component from being defined as:

function AccessibilityIcon(props) {
  var svgDataByHeight = { "16": { "width": 16, "path": "<path fill-rule=\"evenodd\" d=\"M9.923 5.302a3 3 0 10-3.847 0A2.713 2.713 0 005.9 5.5H2A.75.75 0 002 7h3.3l-.578 5.163-.362 2.997a.75.75 0 101.49.18L6.132 13h3.736l.282 2.34a.75.75 0 101.49-.18l-.362-2.997L10.7 7H14a.75.75 0 000-1.5h-3.899a2.697 2.697 0 00-.178-.198zM9.5 3a1.5 1.5 0 11-3 0 1.5 1.5 0 013 0zm-.3 4.073l.495 4.427h-3.39l.496-4.427a1.207 1.207 0 012.398 0z\"></path>" } };
  return React.createElement('svg', getSvgProps(_extends({}, props, { svgDataByHeight: svgDataByHeight })));
}

AccessibilityIcon.defaultProps = {
  className: 'octicon octicon-accessibility',
  size: 16,
  verticalAlign: 'text-bottom'
};

To the following:

const AccessibilityIcon = /*#__PURE__*/ createIconComponent("AccessibilityIcon", "octicon octicon-accessibility", () => ({
  "16": {
    "width": 16,
    "path": /*#__PURE__*/React.createElement("path", {
      fillRule: "evenodd",
      d: "M9.923 5.302a3 3 0 10-3.847 0A2.713 2.713 0 005.9 5.5H2A.75.75 0 002 7h3.3l-.578 5.163-.362 2.997a.75.75 0 101.49.18L6.132 13h3.736l.282 2.34a.75.75 0 101.49-.18l-.362-2.997L10.7 7H14a.75.75 0 000-1.5h-3.899a2.697 2.697 0 00-.178-.198zM9.5 3a1.5 1.5 0 11-3 0 1.5 1.5 0 013 0zm-.3 4.073l.495 4.427h-3.39l.496-4.427a1.207 1.207 0 012.398 0z"
    })
  }
}));

Under the hood, createIconComponent returns the AccessibilityIcon component. This function brings over the existing getSvgProps behavior and bakes it into the returned component.

How it works

The default lib/build/data.json file now includes an AST of the SVG asset. The icon builder in lib/octicons_react converts this svg AST to JS which is included in the svgData argument of createIconComponent.

createIconComponent creates the component used by React and inlines the existing logic from getSvgData into the component.

The notable change at this point is that the path value is no longer a string but is instead a JSXElement (or JSXFragment) which can be set as the children value of the <svg> element.

Testing & Reviewing

I updated the Next.js project to work with the package format so it should allow a preview of the new icon components!

joshblack avatar Sep 19 '22 20:09 joshblack

⚠️ No Changeset found

Latest commit: 999060f0287f746d17ee740ccf7d0b5195c1a882

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar Sep 19 '22 20:09 changeset-bot[bot]

Note: we should make sure https://github.com/primer/octicons/pull/841 is merged before this PR in order to make sure no breaking changes occur

joshblack avatar Sep 19 '22 20:09 joshblack

@colebemis good call on adding that individual test for tree shaking, it seems like React.Fragment is not considered pure (even when used in a pure React.createElement call) and is causing icons that use it to be included 🤔

I updated the approach to include two things:

  • A /*#__PURE__*/ annotation on the createIconComponent function call
  • A getter for the svgDataByHeight object

The getter allows things in that top scope to be tree-shaken since the impure React.Fragment now is only accessed if the function is called.

It seems like the size of the export reduced by ~1kB so tree-shaking is working as-intended with no public API changes (phew 😅)

joshblack avatar Sep 20 '22 16:09 joshblack