IconNames is undefined in unit tests

[Pajn]

Bug Report

• Package version(s): 5.79.1
• Browser and OS versions: N/A, OSX HIgh Sierra

Priorities and help requested (not applicable if asking question):

Are you willing to submit a PR to fix? No

Requested priority: Blocking

Products/sites affected: Not yet public

Describe the issue:

IconNames from @uifabric/icons is undefined in our unit tests which makes it impossible to create any unit test in code which uses fabric icons. This occurs event if we set up the icons with initializeIcons in a Jest setupFile.

Actual behavior:

IconNames is undefined

Expected behavior:

IconNames is not undefined so that code can refer to icons in IconNames without crashing due to reading a property of undefined.

If applicable, please provide a codepen repro:

N/A

[michaelangotti]

@Pajn Could supply a repro or code sample?

[dzearing]

This is easy to repro.

create-react-app my-app --scripts-version=react-scripts-ts
yarn add office-ui-fabric-react @types/prop-types prop-types @uifabric/icons
yarn start

In the App.tsx, use IconNames:

import { IconNames } from '@uifabric/icons/lib/IconNames';
import { Icon } from 'office-ui-fabric-react';

...
        <Icon iconName={IconNames.Download} />

Will compile just fine, but at runtime IconNames is undefined. This is because it is marked as a const enum, and TypeScript doesn't want to auto expand const enums.

You can see the declaration being correctly defined in @uifabric/icons/lib/IconNames.d.ts. However, TypeScript is not consuming the const enum correctly. The expectation is that TypeScript expands IconNames.Download to the literal string "Download".

@RyanCavanaugh might know what to do here.

[dzearing]

If we remove the const from the enum definition, or mark the project in tsconfig.json to preserve const enums, it ends up causing you to pull in 5k of definitions. This is not acceptable.

Another possibility is to export each individual name as an individual const... they will treeshake out properly once we convert the lib folder to es6 in 6.0. But I don't feel super great about that idea.

[RyanCavanaugh]

This compiles correctly if you use tsc.

The most likely problem here is that ts-loader is using ts.transpile or equivalent (e.g. Babel?) to do emit, which means you are not allowed to use const enums because TypeScript is not loading in the imports so it doesn't know that IconNames.Download refers to a const enum.

Assuming that's the case, then whoever configured this starter project should have put isolatedModules: true in the config file.

It's also possible that ts-loader is incorrectly getting the dependencies loaded in.

Another possibility is to export each individual name as an individual const... they will treeshake out properly once we convert the lib folder to es6 in 6.0. But I don't feel super great about that idea.

Honestly const enum with string is a code smell to me and I'm kind of sad TypeScript even has the feature. Obviously we have to think about JavaScript consumers when writing a library, so any JS consumer has to be able to write "Download" instead of IconNames.Download, which means you can't really ever change that string anyway... so why not just make TypeScript authors write "Download" too and stop depending on transpile-time magic? It gets typechecked equally well, autocompleted equally well, etc.. Just my opinion though.

[Pajn]

This is in unit tests so it is unrelated to Webpack or ts-loader. However I guess most of what you said can be read anyway by just replacing ts-loader with ts-jest.

I don't want to have isolatedModules: true just to be able to unit test the project. The project works fine without it and I have never before seen a module that requires it just to be able to write unit tests.

[dzearing]

@RyanCavanaugh Right now, users do type "download" for the icon name.

<Icon name='download'/>

But they want intellisense:

<Icon name={ IconNames.Download } />

So that's why the const enum; we just want the IconNames.Download to be replaced with the string literal.

Hmmm there has to be something you can do here with ts-jest.

[dzearing]

ts-jest issue:
kulshekhar/ts-jest#112

Related note:
https://github.com/kulshekhar/ts-jest/blob/master/README.md#const-enum-is-not-supported

I don't see any workaround here other than:

Option 1: export the icons names as consts;

export const Download = 'download';

Option 2: a full on object (nicer from the autocomplete perspective, worse because you get the whole thing and all the unused entries won't tree shake out):

export const IconNames = {
  Download: 'download'
};

Option 3: a non-const enum (which is even worse because of no tree shaking, plus enums are twice as heavy as plain objects):

export enum IconNames {
  Download = 'download'
}

[dzearing]

Another idea is to add a non-const, non exported enum to the package, so that at the ts-jest layer you can alias @uifabric/lib/IconNames to @uifabric/lib/FullIconNames. It's a little bit extra friction but maybe better than nothing.

[RyanCavanaugh]

TypeScript already provides Intellisense when you're typing a string in a place where the contextual type is a string literal type, so you don't really gain a whole lot with using a symbolic name instead.

[dzearing]

@cliffkoh is there a way we can adjust the typings of Icons? I am a bit confused on the right way to do this.

[micahgodbolt]

Wanted to give this one a little nudge to see if there's any ideas on how we can resolve this.

[kenotron]

@dzearing, there's an internal email thread about this with the OneDrive & Gearbox folks. I ended my last part asking for an opinion from you:

"Given the usage, I am proposing that we end up deprecating the IconNames enum for 7.x. For now, we should tag that object as @deprecated to discourage use. @dzearing – what’s your opinion?"

In the meantime, internally within the Fabric codebase, we should remove the use of these IconNames enum altogether and just use strings like the library consumers should. So then the unit tests would be an issue.

[kenotron]

@dzearing - I took a look at our codebase and can't find usage of the IconNames enum anymore except inside a visual regression test story for Nav. (I searched for "IconNames."). I think this bug is actually addressed.

@Pajn - it seems that ts-jest now has isolatedModules default to false, I think that jest would ts-jest would work with this const enum output for now. I don't think Fabric team will action on this other than adding deprecation warnings on the IconNames enum. Please close this bug if you feel that your issue as been addressed.

[msft-github-bot]

This issue has been automatically marked as stale because it has marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. Thank you for your contributions to Fabric React!