Adds JLIcon, an easier way to consume icons as SVG nodes

[telamonian]

References

Followup to #6034, #7192, #7266, and a bunch of others

Code changes

Working on the one true way to consume icons in jlab, with a goal to get this in by v2.0. The basic design plan is this:

• Create a wrapper that will take the raw contents of an .svg file (as a string) and return a React.Component class
• There will be one component class per icon (ie per .svg file)
• Each component class will also have a static method to allow for the use of it's specific icon outside of jsx/react
• Creating new icon classes will be as easy as importing your .svg then making a call to the wrapper

User-facing changes

None

Backwards-incompatible changes

Will remove all other means of consuming icons. In exchange, will add a super easy way to setup/consume theme-aware icons in extensions.

[jupyterlab-dev-mode]

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link:

[telamonian]

@saulshanabrook Saul said:

Another option is we could move from having a IconRegistry to just exporting each icon as a typescript constant. We could have an Icon class and instantiate it for each icon, for something like:

import runningSVG form './icons/running.svg'
# iconSvg is the text source of the icon. We could setup webpack to always load SVG with the >`raw-loader`
export const runningIcon = new Icon(runningSVG)

Then in another file you could import the icon and get the react component or HTML node creator off of it. This would let us rely on TypeScript to make sure we define each icon we want and also it eliminates the need to learn a new concept of an IconRegistry.

In this case, we would also remove the icon extension.

The design goal of this PR can be thought of as a riff on Saul's suggestion.

[telamonian]

The return type of createIcon is the union of the return type of React.forwardRef plus whatever static methods that I add (currently just element). I initially tried to declare the return type as IComponent, using typeof and ReturnType to capture the return type of forwardRef:

  export interface IComponent extends ReturnType<typeof React.forwardRef<HTMLDivElement, JLIcon.IProps>> {
    element?: (props: IProps) => HTMLElement;
  }

but that's not valid syntax, due to the vagaries of the current typeof implementation. You can get something that is valid syntax by assigning a type alias to the result of typeof:

  type _ret<T, P> = typeof React.forwardRef;
  export interface IComponent extends ReturnType<_ret<HTMLDivElement, JLIcon.IProps>> {
    element?: (props: IProps) => HTMLElement;
  }

but for reasons I don't understand, the concrete template parameters in ReturnType<_ret<HTMLDivElement, JLIcon.IProps>> don't actually end up getting bound correctly to the resulting type. Finally, I just gave up and copied the return type of forwardRef directly from @types/react:

  export interface IComponent extends ForwardRefExoticComponent<PropsWithoutRef<IProps> & RefAttributes<HTMLDivElement>> {
    element?: (props: IProps) => HTMLElement;
  }

This works well, though it seems kind of fragile (wrt changes to @types/react). The issue here has something to do with using both typeof and ReturnType on a generic function, so if anyone has any insight into the inner workings of Typescript in this case it would be most welcome.

[sccolbert]

I think I like Saul's quoted idea above. Create an Icon class, that has some methods on it which return the content in various forms. One method could be component() which returns the react component. That will be easier to specify the typing, and more flexible in the future should you decide you need to consume the icon another way.

<span>myIcon.component()</span>

[telamonian]

Whelp, apparently I was under some mistaken impressions about valid syntax for JSX component names; I thought they had to start with an uppercase letter and have no dot. In actual fact, the only invalid names seem to be those that start with a lowercase letter but don't have a dot:

const bar = TrustedIcon;
const Bar = TrustedIcon;

const foo: any = {};
const Foo = foo;
foo.icon = TrustedIcon;
foo.Icon = TrustedIcon;

const barFunc = () => TrustedIcon;
const BarFunc = () => TrustedIcon;

foo.iconFunc = () => TrustedIcon;
foo.IconFunc = () => TrustedIcon;

return (
  <div style={{display: "inline-flex"}}>
    <bar/>       {/* error! */}
    <Bar/>       {/* valid */}

    <foo.icon/>  {/* valid */}
    <foo.Icon/>  {/* valid */}
    <Foo.icon/>  {/* valid */}
    <Foo.Icon/>  {/* valid */}

    {/* can't use a function call as a JSX component */}

    <barFunc()/>       {/* error! */}
    <BarFunc()/>       {/* error! */}

    <foo.iconFunc()/>  {/* error! */}
    <foo.IconFunc()/>  {/* error! */}
    <Foo.iconFunc()/>  {/* error! */}
    <Foo.IconFunc()/>  {/* error! */}

    {/* but you can use function calls if you escape from JSX */}

    {barFunc()}                                           {/* valid */}
    {React.createElement(barFunc(), {className: "bar"})}  {/* valid */}
  </div>

Function calls are never valid JSX components, but you can use them if you escape from JSX.

[telamonian]

@sccolbert I'll take your suggestion (to take Saul's original suggestion) and rearrange JLIcon into a class

[telamonian]

@saulshanabrook @sccolbert Thank you for your feedback. It helped me to figure out how to vastly simplify the design of the new icon objects and still get the ease-of-use in JSX that I was chasing after.

In the latest changes that I just pushed, all of the icon objects are now instances of the JLIcon class. JLIcon instances have an .element method that returns an HTMLElement, and a react property that holds a React component. Here's an example of a JLIcon instance being used in JSX/React:

jupyterlab/packages/notebook/src/truststatus.tsx

Lines 47 to 51 in 006b57a

	if (props.allCellsTrusted) {
	return <trustedIcon.react top={'2px'} kind={'statusBar'} />;
	} else {
	return <notTrustedIcon.react top={'2px'} kind={'statusBar'} />;
	}

Though the internals of JLIcon need some work (and possibly its interface needs a few tweaks as well), I'm happy with the current direction. It completely eliminates the need for the convoluted type declarations of the previous design.

The next thing for me to tackle will be the non-React use cases for JLIcon, including figuring out the best way to integrate it with Phosphor.

[telamonian]

The JLIcon class is finished, so I think this PR is as well. There's more work to do in terms of switching everything over to JLIcon and cleaning up several layers worth of old icon API, but I think it would be best to get JLIcon merged in and then follow up with another PR.

Before this can be pulled in we'll need to do another lumino release, since this PR relies on unreleased changes in the latest master version of @lumino/virtualdom. @blink1073 Is there anything I can do to help out with that?

[saulshanabrook]

I will review this today! Thanks @telamonian for all your work here.

[telamonian]

Thanks @saulshanabrook!

If you actually want to test the code from this PR, you'll also need to yarn link in the latest master of Lumino, with the changes from jupyterlab/lumino#36. The yarn link instructions are here. I ended up needing to link in 4 packages to get everything to work correctly:

• @lumino/commands
• @lumino/messaging
• @lumino/virtualdom
• @lumino/widgets

[saulshanabrook]

@telamonian Great, I will have a go at running it locally. Looks good overall! I wanna do some tests around the React component stuff... Curious if I can factor out the component into an external function.

[telamonian]

@saulshanabrook Looking over your comments so far, it might be better if you just take a look at #7700. It's the direct continuation of this PR, and already has resolutions for most the things you pointed out in the review. For example, I dropped the use of render/unrender from listings.

[jasongrout]

Since #7700 includes this PR and important fixes for this PR, should we just close this PR in deference to that one?

[telamonian]

Since #7700 includes this PR and important fixes for this PR, should we just close this PR in deference to that one?

@jasongrout I think that's fine. I originally split this into two PRs in order to make it easier to read over and review the basic code for JLIcon. I personally find it very difficult to review any PR that balloons over the ~1000 lines mark. Not sure if this approach actually made it any easier, since I'm not really in a position to accept changes on this PR.

So as long as someone is up to reviewing #7700 as one big piece (it currently has changes to 239 files and over 4000 lines), feel free to close #7299

[jasongrout]

@saulshanabrook - since you are reviewing this (thanks again!), how about you make the decision about whether to review this PR or close it and just look at #7700.

[saulshanabrook]

Let's close this and just look at that one.