No tree shaking on import of octicons

[elisherer]

import of octicons adds ~800KB to build assets
The 5 icons are expected to be imported alone and not import the rest of the icons from that library.

Workaround:

I copied the file and replaced the alerts to the following: (octicons are licensed MIT anyway, so it can just be added in an important comment instead)

alerts: [
      {
        keyword: "NOTE",
        icon: `<svg class="octicon octicon-info mr-2" viewBox="0 0 16 16" version="1.1" width="16" height="16" aria-hidden="true"><path d="M0 8a8 8 0 1 1 16 0A8 8 0 0 1 0 8Zm8-6.5a6.5 6.5 0 1 0 0 13 6.5 6.5 0 0 0 0-13ZM6.5 7.75A.75.75 0 0 1 7.25 7h1a.75.75 0 0 1 .75.75v2.75h.25a.75.75 0 0 1 0 1.5h-2a.75.75 0 0 1 0-1.5h.25v-2h-.25a.75.75 0 0 1-.75-.75ZM8 6a1 1 0 1 1 0-2 1 1 0 0 1 0 2Z"/></svg>`,
        title: "Note",
      },
      {
        keyword: "IMPORTANT",
        icon: `<svg class="octicon octicon-report mr-2" viewBox="0 0 16 16" version="1.1" width="16" height="16" aria-hidden="true"><path d="M0 1.75C0 .784.784 0 1.75 0h12.5C15.216 0 16 .784 16 1.75v9.5A1.75 1.75 0 0 1 14.25 13H8.06l-2.573 2.573A1.458 1.458 0 0 1 3 14.543V13H1.75A1.75 1.75 0 0 1 0 11.25Zm1.75-.25a.25.25 0 0 0-.25.25v9.5c0 .138.112.25.25.25h2a.75.75 0 0 1 .75.75v2.19l2.72-2.72a.749.749 0 0 1 .53-.22h6.5a.25.25 0 0 0 .25-.25v-9.5a.25.25 0 0 0-.25-.25Zm7 2.25v2.5a.75.75 0 0 1-1.5 0v-2.5a.75.75 0 0 1 1.5 0ZM9 9a1 1 0 1 1-2 0 1 1 0 0 1 2 0Z"/></svg>`,
        title: "Important",
      },
      {
        keyword: "WARNING",
        icon: `<svg class="octicon octicon-alert mr-2" viewBox="0 0 16 16" version="1.1" width="16" height="16" aria-hidden="true"><path d="M6.457 1.047c.659-1.234 2.427-1.234 3.086 0l6.082 11.378A1.75 1.75 0 0 1 14.082 15H1.918a1.75 1.75 0 0 1-1.543-2.575Zm1.763.707a.25.25 0 0 0-.44 0L1.698 13.132a.25.25 0 0 0 .22.368h12.164a.25.25 0 0 0 .22-.368Zm.53 3.996v2.5a.75.75 0 0 1-1.5 0v-2.5a.75.75 0 0 1 1.5 0ZM9 11a1 1 0 1 1-2 0 1 1 0 0 1 2 0Z"/></svg>`,
        title: "Warning",
      },
      {
        keyword: "TIP",
        icon: `<svg class="octicon octicon-light-bulb mr-2" viewBox="0 0 16 16" version="1.1" width="16" height="16" aria-hidden="true"><path d="M8 1.5c-2.363 0-4 1.69-4 3.75 0 .984.424 1.625.984 2.304l.214.253c.223.264.47.556.673.848.284.411.537.896.621 1.49a.75.75 0 0 1-1.484.211c-.04-.282-.163-.547-.37-.847a8.456 8.456 0 0 0-.542-.68c-.084-.1-.173-.205-.268-.32C3.201 7.75 2.5 6.766 2.5 5.25 2.5 2.31 4.863 0 8 0s5.5 2.31 5.5 5.25c0 1.516-.701 2.5-1.328 3.259-.095.115-.184.22-.268.319-.207.245-.383.453-.541.681-.208.3-.33.565-.37.847a.751.751 0 0 1-1.485-.212c.084-.593.337-1.078.621-1.489.203-.292.45-.584.673-.848.075-.088.147-.173.213-.253.561-.679.985-1.32.985-2.304 0-2.06-1.637-3.75-4-3.75ZM5.75 12h4.5a.75.75 0 0 1 0 1.5h-4.5a.75.75 0 0 1 0-1.5ZM6 15.25a.75.75 0 0 1 .75-.75h2.5a.75.75 0 0 1 0 1.5h-2.5a.75.75 0 0 1-.75-.75Z"/></svg>`,
        title: "Tip",
      },
      {
        keyword: "CAUTION",
        icon: `<svg class="octicon octicon-stop mr-2" viewBox="0 0 16 16" version="1.1" width="16" height="16" aria-hidden="true"><path d="M4.47.22A.749.749 0 0 1 5 0h6c.199 0 .389.079.53.22l4.25 4.25c.141.14.22.331.22.53v6a.749.749 0 0 1-.22.53l-4.25 4.25A.749.749 0 0 1 11 16H5a.749.749 0 0 1-.53-.22L.22 11.53A.749.749 0 0 1 0 11V5c0-.199.079-.389.22-.53Zm.84 1.28L1.5 5.31v5.38l3.81 3.81h5.38l3.81-3.81V5.31L10.69 1.5ZM8 4a.75.75 0 0 1 .75.75v3.5a.75.75 0 0 1-1.5 0v-3.5A.75.75 0 0 1 8 4Zm0 8a1 1 0 1 1 0-2 1 1 0 0 1 0 2Z"/></svg>`,
        title: "Caution",
      },
    ],

[yukidaruma]

Looks like this issue is specific to esbuild (I could reproduce it by running esbuild with --bundle option in this repo), or at least it's not happening with Vite. (See: evanw/esbuild#1241)

I'm gonna make a PR to fix the issue.

[elisherer]

I would also like to not have hast-util-from-html as a dependency since the other parts of this library creates the hast structure on its own (see defaultBuild(alertOptions, originalChildren).

suggested replacement:

function octiconFactory(name: string, path: string): Element {
  return {
    type: "element",
    tagName: "svg",
    properties: {
      className: ["octicon", name, "mr-2"],
      viewBox: "0 0 16 16",
      version: "1.1",
      width: "16",
      height: "16",
      ariaHidden: "true",
    },
    children: [
      {
        type: "element",
        tagName: "path",
        properties: {
          d: path,
        },
        children: [],
        position: { start: { line: 1, column: 116, offset: 115 }, end: { line: 1, column: 375, offset: 374 } },
      },
    ],
    position: { start: { line: 1, column: 1, offset: 0 }, end: { line: 1, column: 381, offset: 380 } },
  };
}
//...
  alerts: [
      {
        keyword: "NOTE",
        icon: octiconFactory(
          "octicon-info",
          "M0 8a8 8 0 1 1 16 0A8 8 0 0 1 0 8Zm8-6.5a6.5 6.5 0 1 0 0 13 6.5 6.5 0 0 0 0-13ZM6.5 7.75A.75.75 0 0 1 7.25 7h1a.75.75 0 0 1 .75.75v2.75h.25a.75.75 0 0 1 0 1.5h-2a.75.75 0 0 1 0-1.5h.25v-2h-.25a.75.75 0 0 1-.75-.75ZM8 6a1 1 0 1 1 0-2 1 1 0 0 1 0 2Z",
        ),
        title: "Note",
      },
      {
        keyword: "IMPORTANT",
        icon: octiconFactory(
          "octicon-report",
          "M0 1.75C0 .784.784 0 1.75 0h12.5C15.216 0 16 .784 16 1.75v9.5A1.75 1.75 0 0 1 14.25 13H8.06l-2.573 2.573A1.458 1.458 0 0 1 3 14.543V13H1.75A1.75 1.75 0 0 1 0 11.25Zm1.75-.25a.25.25 0 0 0-.25.25v9.5c0 .138.112.25.25.25h2a.75.75 0 0 1 .75.75v2.19l2.72-2.72a.749.749 0 0 1 .53-.22h6.5a.25.25 0 0 0 .25-.25v-9.5a.25.25 0 0 0-.25-.25Zm7 2.25v2.5a.75.75 0 0 1-1.5 0v-2.5a.75.75 0 0 1 1.5 0ZM9 9a1 1 0 1 1-2 0 1 1 0 0 1 2 0Z",
        ),
        title: "Important",
      },
      {
        keyword: "WARNING",
        icon: octiconFactory(
          "octicon-alert",
          "M6.457 1.047c.659-1.234 2.427-1.234 3.086 0l6.082 11.378A1.75 1.75 0 0 1 14.082 15H1.918a1.75 1.75 0 0 1-1.543-2.575Zm1.763.707a.25.25 0 0 0-.44 0L1.698 13.132a.25.25 0 0 0 .22.368h12.164a.25.25 0 0 0 .22-.368Zm.53 3.996v2.5a.75.75 0 0 1-1.5 0v-2.5a.75.75 0 0 1 1.5 0ZM9 11a1 1 0 1 1-2 0 1 1 0 0 1 2 0Z",
        ),
        title: "Warning",
      },
      {
        keyword: "TIP",
        icon: octiconFactory(
          "octicon-light-bulb",
          "M8 1.5c-2.363 0-4 1.69-4 3.75 0 .984.424 1.625.984 2.304l.214.253c.223.264.47.556.673.848.284.411.537.896.621 1.49a.75.75 0 0 1-1.484.211c-.04-.282-.163-.547-.37-.847a8.456 8.456 0 0 0-.542-.68c-.084-.1-.173-.205-.268-.32C3.201 7.75 2.5 6.766 2.5 5.25 2.5 2.31 4.863 0 8 0s5.5 2.31 5.5 5.25c0 1.516-.701 2.5-1.328 3.259-.095.115-.184.22-.268.319-.207.245-.383.453-.541.681-.208.3-.33.565-.37.847a.751.751 0 0 1-1.485-.212c.084-.593.337-1.078.621-1.489.203-.292.45-.584.673-.848.075-.088.147-.173.213-.253.561-.679.985-1.32.985-2.304 0-2.06-1.637-3.75-4-3.75ZM5.75 12h4.5a.75.75 0 0 1 0 1.5h-4.5a.75.75 0 0 1 0-1.5ZM6 15.25a.75.75 0 0 1 .75-.75h2.5a.75.75 0 0 1 0 1.5h-2.5a.75.75 0 0 1-.75-.75Z",
        ),
        title: "Tip",
      },
      {
        keyword: "CAUTION",
        icon: octiconFactory(
          "octicon-stop",
          "M4.47.22A.749.749 0 0 1 5 0h6c.199 0 .389.079.53.22l4.25 4.25c.141.14.22.331.22.53v6a.749.749 0 0 1-.22.53l-4.25 4.25A.749.749 0 0 1 11 16H5a.749.749 0 0 1-.53-.22L.22 11.53A.749.749 0 0 1 0 11V5c0-.199.079-.389.22-.53Zm.84 1.28L1.5 5.31v5.38l3.81 3.81h5.38l3.81-3.81V5.31L10.69 1.5ZM8 4a.75.75 0 0 1 .75.75v3.5a.75.75 0 0 1-1.5 0v-3.5A.75.75 0 0 1 8 4Zm0 8a1 1 0 1 1 0-2 1 1 0 0 1 0 2Z",
        ),
        title: "Caution",
      },
    ],

And this

    const alertIcon = fromHtml(alertOptions.icon, { fragment: true }).children[0];

should be removed

[chrisweb]

@elisherer I am not sure I understand, where is the three shaking problem occuring, in your app itself? If so, what builder do you use?

I wonder because (I verified just now), the package build itself (rehype-github-alerts) is 10k: https://www.npmjs.com/package/rehype-github-alerts?activeTab=code

@yukidaruma not sure what PR you want to do, but please consider that adding the icons as SVG into code is actually increasing the package for everyone, even users that replace the octicons with custom icons (we discussed this already in another issue). so let's make sure we think about potential drawbacks, other alternatives and then make a pros/cons comparison of each solution

[elisherer]

Yeah @chrisweb , I wasn't talking about the package size but about the octicons dependency which doesny support tree shaking (as @yukidaruma found out).
I was suggesting as a "double win" to remove dependencies from the lib (both octicons and html to hast) in a very small price of those 5 icons, embedded. Your call of course.

[chrisweb]

which doesn't support tree shaking

to my knowledge none do, that is the reason why Next.js has a new experimental feature called "optimizePackageImports", if you look at the default packages on that list you will notice that almost half are "icon" packages: https://nextjs.org/docs/app/api-reference/config/next-config-js/optimizePackageImports

I was suggesting as a "double win"

it is good to question things, especially when related to performance (like this in case), the problem with the icons is that (in my opinion) it not a so black and white (its a shade of grey), we already discussed the icons back then, when the project was mostly "unstyled" in ticket 12. I had lookws into other packages to see how they handle icons and css files and at that time having the icons as dependency seemed like a good solution.

As I explained previously (not sure it was discord or a ticket), I agree that in most cases packages should always be unstyled, to make it easy to embed them into your project and adjust the styling to your existing style guide, however for package the context is different, the goal of this package is to make the alerts you added to markdown documents on github look the same when they get rendered in your project. This is why the icons got added, it is because a dev that sees "github-alerts" will expect them to work and look like the alerts on github. There are many other "alerts" / "callouts" / "admonitions" packages available on npm, that are not styled (as they don't pretend to be "github like").

Then there is the "official" rehype github alert package which to my knowledge has a reduced amount of dependencies, not sure if you (@elisherer) did test it as replacement, maybe it suits your needs better?

I will keep the discussion open, feel free to discuss this further (this is not a "prove me wrong" thread, its ok to disagree, but also if new ideas come up then I might reconsider it), to honest for now I see no need to change something, also right now I am happy that package has been stable for a while (not requiring updates)