Large front-end bundles?

blueputty01 commented 1 year ago

Hello!

First, this is an amazing package, and makes the CMS work so much easier!

This could be a misconfiguration on my end, but it looks like the bundle sizes of the react code from the example are very large. It looks like the entire icon package is served.

Perhaps a version of this lazy loading code may be suggested instead?

          const ComponentA = dynamic(() => {
            switch (icon.provider) {
              case 'mdi':
                return import(`react-icons/md`).then((mod) => mod[icon.name!]);
              case 'fa':
                return import(`react-icons/fa`).then((mod) => mod[icon.name!]);
              default:
                return import(`react-icons/fa`).then(
                  (mod) => mod.FaMapMarkerAlt
                );
            }
          });

Thank you for writing this plugin :)

blueputty01 commented 1 year ago

Closed, this is more of an issue with react-icons not being tree-shakeable.

blueputty01 commented 1 year ago

What if an SVG was set as a part of the data as well to make this more front-end language-agnostic & avoid large builds?

Eg:

  const setIcon: SearchResultsOnSelectCallback = (icon) => {
    if (selected && icon.name === selected.name) return unsetIcon();

    onChange([
      setIfMissing({
        _type: schemaType.name,
      }),
      set(icon.name, ['name']),
      set(icon.provider, ['provider']),
      set(renderToString(icon.component()), ['svg']), // <-- addition
    ]);

christopherafbjur commented 1 year ago

Hi @blueputty01! Indeed it doesn't seem like there is much that can be done regarding react-icons. How ever your last proposal is actually something I've been thinking about. Might be something we can implement that could be toggled on with an option. Feel free to create a PR. I have limited time at this point but will try to find time to stay more active with this lib asap 😊

blueputty01 commented 1 year ago

Hi @christopherafbjur! I'll look into this more and open a PR hopefully next weekend.

Reopening since I think that the link to the PR will be more clear. Thanks again for this plugin!

christopherafbjur commented 1 year ago

Tested your proposed solution @blueputty01 but it seems that it requires configuring vite with polyfills which is probably not the job for this plugin (I assume you were thinking of using renderToString method from react-dom/server dependency). But feel free to correct me if I'm wrong :)

Another solution that I tested and that seem to work is:

Plugin

import {format, plugins} from 'pretty-format';
import renderer from 'react-test-renderer';
...
...
const setIcon: SearchResultsOnSelectCallback = (icon) => {
    if (selected && icon.name === selected.name) return unsetIcon()

    const svgString = format(renderer.create(icon.component()).toJSON(), {
      plugins: [plugins.ReactTestComponent],
      printFunctionName: false,
    }).replace(/\s+|\n/gm, ' ')
    // <svg fill=\"none\" height=\"1em\" stroke=\"currentColor\" strokeLinecap=\"round\" strokeLinejoin=\"round\" strokeWidth=\"2\" style={ Object { \"color\": undefined, } } viewBox=\"0 0 24 24\" width=\"1em\" xmlns=\"http://www.w3.org/2000/svg\" > <circle cx=\"12\" cy=\"12\" r=\"10\" /> <polyline points=\"8 12 12 16 16 12\" /> <line x1=\"12\" x2=\"12\" y1=\"8\" y2=\"16\" /> </svg>

    onChange([
      setIfMissing({
        _type: schemaType.name,
      }),
      set(icon.name, ['name']),
      set(icon.provider, ['provider']),
      set(svgString, ['svg']),
    ])

    return setSelected(icon)
  }

Client

import SVG from 'react-inlinesvg';

const sanityResponse = {
    "_type": "iconPicker",
    "name": "arrow-down-circle",
    "provider": "fi",
    "svg": "<svg fill=\"none\" height=\"1em\" stroke=\"currentColor\" strokeLinecap=\"round\" strokeLinejoin=\"round\" strokeWidth=\"2\" style={ Object { \"color\": undefined, } } viewBox=\"0 0 24 24\" width=\"1em\" xmlns=\"http://www.w3.org/2000/svg\" > <circle cx=\"12\" cy=\"12\" r=\"10\" /> <polyline points=\"8 12 12 16 16 12\" /> <line x1=\"12\" x2=\"12\" y1=\"8\" y2=\"16\" /> </svg>"
  }
...
<SVG src={sanityResponse.svg} />

blueputty01 commented 1 year ago

@christopherafbjur Thanks for taking a look at this! I was indeed thinking of the function from react-dom/server. Your solution is more clean.

Issue:

The sample client component works well in my tests.

Using encodeURIComponent to populate the src attribute on an img tag using the generated SVG markup doesn't seem to work as well.

It seems like the parser gets confused by the curly braces in the style attribute: style={ Object { \"color\": undefined, } }

Proposal:

A regex expression eg \{.*\} to remove objects from the final string
Removing the style key/any keys with object type values from the react-test-renderer object before passing it to pretty-format
Possibly leave it as an upstream/downstream issue...?

Issue example:

For example, the fi alert circle passed into encodeURIComponent produces the error: error on line 1 at column 120: AttValue: " or ' expected

Generated SVG: <svg fill="none" height="1em" stroke="currentColor" strokeLinecap="round" strokeLinejoin="round" strokeWidth="2" style={ Object { "color": undefined, } } viewBox="0 0 24 24" width="1em" xmlns="http://www.w3.org/2000/svg" > <circle cx="12" cy="12" r="10" /> <line x1="12" x2="12" y1="8" y2="12" /> <line x1="12" x2="12.01" y1="16" y2="16" /> </svg>

src attr: data:image/svg+xml;utf8,%3Csvg%20fill%3D%22none%22%20height%3D%221em%22%20stroke%3D%22currentColor%22%20strokeLinecap%3D%22round%22%20strokeLinejoin%3D%22round%22%20strokeWidth%3D%222%22%20style%3D%7B%20Object%20%7B%20%22color%22%3A%20undefined%2C%20%7D%20%7D%20viewBox%3D%220%200%2024%2024%22%20width%3D%221em%22%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20%3E%20%3Ccircle%20cx%3D%2212%22%20cy%3D%2212%22%20r%3D%2210%22%20%2F%3E%20%3Cline%20x1%3D%2212%22%20x2%3D%2212%22%20y1%3D%228%22%20y2%3D%2212%22%20%2F%3E%20%3Cline%20x1%3D%2212%22%20x2%3D%2212.01%22%20y1%3D%2216%22%20y2%3D%2216%22%20%2F%3E%20%3C%2Fsvg%3E

christopherafbjur commented 1 year ago

@blueputty01 Thank you for digging deeper into this. Your proposal of removing the style object before passing it to pretty-format seems like the most clean solution to me.

How ever I just discovered another problem with the parsing of pretty-format which cause inconsistencies in the original SVG (from react icons) and the one saved to later be used in the client.

Issue:

Using fi alert circle

Left icon is the parsed svg string after running it through pretty-format, right icon is the original one from react-icons.

Comparison:

Left (parsed)

<svg fill="none" height="1em" stroke="currentColor" strokeLinecap="round" strokeLinejoin="round" strokeWidth="2" viewBox="0 0 24 24" width="1em" xmlns="http://www.w3.org/2000/svg"><circle cx="12" cy="12" r="10" /><line x1="12" x2="12" y1="8" y2="12" /><line x1="12" x2="12.01" y1="16" y2="16" /></svg>

Right (original)

<svg stroke="currentColor" fill="none" stroke-width="2" viewBox="0 0 24 24" stroke-linecap="round" stroke-linejoin="round" style="width: 1.5em; height: 1em;" height="1em" width="1em" xmlns="http://www.w3.org/2000/svg"><circle cx="12" cy="12" r="10"></circle><line x1="12" y1="8" x2="12" y2="12"></line><line x1="12" y1="16" x2="12.01" y2="16"></line></svg>

The difference in attribute order when comparing the two made it quite difficult to see the issue, but the issue is the attribute casing for some attributes where it's using kebab casing for the original one (as it should) but using camel casing for the parsed one. As an example strokeLinecap for parsed and stroke-linecap for original.

One possible solution might be to simply convert all occurrences of camel casing to kebab casing but this might also create more problems since, for instance, attribute viewBox is using camel casing in both cases. I will have to do some research on this.

christopherafbjur commented 1 year ago

@blueputty01 Created a draft PR for this. I think this should work. Please let me know what you think and if you see any potential improvements. Not really found of having to define an array of ignores for the camel cased attributes and unaware if all of them are really required for this use case https://github.com/christopherafbjur/sanity-plugin-icon-picker/pull/39

blueputty01 commented 1 year ago

@christopherafbjur I'll be honest, I don't know much about these testing libraries.

However, @testing-library/react seems to avoid all the issues we identified in the generated code (casing, stray objects) with a potentially cleaner implementation:

import { render } from '@testing-library/react';
...
const svgString = render(icon.component).baseElement;

Generated fi alert circle (diff seems to be an additional style object with height and width): <svg stroke="currentColor" fill="none" stroke-width="2" viewBox="0 0 24 24" stroke-linecap="round" stroke-linejoin="round" height="1em" width="1em" xmlns="http://www.w3.org/2000/svg"><circle cx="12" cy="12" r="10"></circle><line x1="12" y1="8" x2="12" y2="12"></line><line x1="12" y1="16" x2="12.01" y2="16"></line></svg>

christopherafbjur commented 1 year ago

@blueputty01 Thank you for this proposal, this was a really neat idea! How ever I was unable to reproduce this using .baseElement when testing in a studio since it was returning the document.body which seem to be the expected behavior (reference). How ever I could reproduce the same svg string if instead using render(icon.component()).container.innerHTML. The problem however is that in both scenarios (using the render method) a div will be appended to document.body to which the icon component will be appended (reference).

And in our case this means that each time we select an icon in the picker, a div will be appended to the document.body since it's never removed. Doesn't seem optimal to pollute the DOM like this.

Example of toggling between 3 different icons from the picker

blueputty01 commented 1 year ago

@christopherafbjur Oops! That's definitely not ideal.

Took a deeper look into your PR, and I don't see any direct improvements I could recommend!

Alternatively, what if we took advantage of the existing element from the sanity UI button? This would resolve our casing issue.

Eg:

This would obviously need additional typings, but I'd be happy to open a PR to add that myself if you'd like.

IconPicker:

  const setIcon: SearchResultsOnSelectCallback = (
    icon: IconObject,
    ele: HTMLButtonElement
  ) => {
    if (selected && icon.name === selected.name) return unsetIcon();

    const svgString = ele.getElementsByTagName('svg')[0].outerHTML; // <--here

SearchResults:

  function IconButton(icon: IconObject) {
    const buttonRef = useRef<HTMLButtonElement>(null);

    return (
      <Button
        ref={buttonRef} // <--here
        key={icon.provider.concat(icon.name)}
        mode="ghost"
        onClick={() => onSelect(icon, buttonRef.current)}
        text={<icon.component />}
        style={{ marginTop: '5px' }}
        selected={
          !!selected &&
          selected.provider === icon.provider &&
          icon.name === selected.name
        }
      />
    );
  }

christopherafbjur commented 1 year ago

@blueputty01 That looks elegant! I haven't tested this but by the looks of it, it should do the trick and should solve the attribute casing things with quite a reduced amount of lines. Feel free to create a PR 🙂

JJK801 commented 1 year ago

any update on it?

i really need it to reduce my frontend package.

maybe i can make the PR if it can help

blueputty01 commented 1 year ago

@JJK801 Thanks for reminding me! The past week was really hectic. I'll put in a PR today :)

blueputty01 commented 1 year ago

@christopherafbjur Created a PR! Let me know your thoughts :)

JJK801 commented 1 year ago

thanks @blueputty01 looks like a smart & sharp solution

christopherafbjur commented 1 year ago

Checked this briefly and it looks great! Will have a closer look tomorrow @blueputty01

christopherafbjur / sanity-plugin-icon-picker