Fluentui: Get rid of the use of const enum inside OUFR

Created on 17 Sep 2018  路  11Comments  路  Source: microsoft/fluentui

Describe the feature that you would like added
This is related to #6185. Even though Typescript supports const enums, it isn't very nice as a library to depend on it because compilation with isolatedModules / transpileOnly would not work with const enums. These flags are required for the likes of ts-loader in order for reasonable innerloop build times.

OUFR has a few offending instances of this:

screen shot 2018-09-17 at 9 32 56 am

This feature request is to find a sensible deprecation plan towards getting rid of this in our codebase.

What component or utility would this be added to

Have you discussed this feature with our team, and if so, who

Additional context/screenshots

Normal Cleanup

Most helpful comment

All 11 comments

I think one of the work artifacts for this issue should be to update the TypeScript guidelines regarding enum usage.

There may be reasons for us to use const enums internally. One thing we can do is do what the TS team does - which is using const enum, combined with preserveEnums, then scurbbing the .d.ts files as a post-build step of all const enums.

This should be a good way to benefit from const enums without having an adverse impact on our downstream consumers.

That's an interesting idea @cliffkoh . There are some cases where we expose const enums externally for Intellisense/quality of life (e.g. IconNames in @uifabric/icons).

In cases like those, it's very convenient to be able to have type-safety of a large item set without the overhead of importing an entire object. I'm not up-to-speed on if there's a decent substitute for that use case--would be curious if anyone has a suggestion.

One consideration here is that Create React App's new built-in TypeScript option doesn't support const enums out of the box:
image

In fact, CRA's build script will actually force isolatedModules to true if you try to turn it off. While we shouldn't bank our decisions entirely on issues like this, it's reasonable to expect that a bunch of new Fabric consumers will encounter friction with CRA + TS + OUIFR while we expose const enums publicly.

In the meantime, I'll remove the Create React App --typescript suggestion from our tutorial (see #6965).

I'd love to get this closed out. It creates friction in random places.

@cliffkoh & I spoke about this - we're coming with a plan to address this issue this way:

  1. we'll write a script to SCRUB out the const in const enum using the same regex approach as Typescript team
  2. create equivalent keyof SomeEnumType type alias to be exported for all instances of const enums that we have exposed as public API - this will produce a set of string union types
  3. promote the use of said string union types inside AND outside of Fabric codebase

I'm going to have to experiment to see if putting (2) in the same file as the const enum would end up making consumers of said file to end up incurring the full import of that file increasing bytes over the wire. If so, then I'll have to split (2) string union type files into a separate file.

I would categorize this as clean up task still, but it seems it would be a bit more work than just a simple fix. I'm going to try and address this in my shield shift during the 2nd week (next week) rather than dealing with it this week.

Update: after some investigation and offline chats with @dzearing & @cliffkoh, I'm going to adjust my approach here:

  1. I'll be converting the existing const enum of small number of values to simple const objects - this will have a minimal API impact (e.g. DirectionalHint)
  2. I'll convert the IconNames enum that seem to be able to grow over time to small set of export const SomeIconName = 'SomeIconName'; - working with treeshaking to lock the size down for just enough bytes to be downloaded for the iconnames by usage.

:tada:This issue was addressed in #7098, which has now been successfully released as @uifabric/[email protected].:tada:

Handy Links:

:tada:This issue was addressed in #7098, which has now been successfully released as @uifabric/[email protected].:tada:

Handy Links:

:tada:This issue was addressed in #7098, which has now been successfully released as @uifabric/[email protected].:tada:

Handy Links:

:tada:This issue was addressed in #7098, which has now been successfully released as [email protected].:tada:

Handy Links:

Was this page helpful?
0 / 5 - 0 ratings

Related issues

just-joshing picture just-joshing  路  35Comments

chrismohr picture chrismohr  路  45Comments

JoshuaKGoldberg picture JoshuaKGoldberg  路  33Comments

ryancole picture ryancole  路  39Comments

JoshuaKGoldberg picture JoshuaKGoldberg  路  33Comments