Fluentui: Can't reference office-ui-fabric-react with isolatedModules turned on

[andyrooger]

Please provide a reproduction of the bug in a codepen:

I don't seem to be able to set up a tsconfig.json in codepen, but I have a simple repro below:

package.json

{
  "name": "fabric-react-repro",
  "version": "0.0.0",
  "private": true,
  "scripts": {
    "build": "tsc -p ."
  },
  "devDependencies": {
    "@types/react": "16.3.16",
    "@types/react-dom": "16.0.6",
    "typescript": "2.9.2"
  },
  "dependencies": {
    "office-ui-fabric-react": "6.61.0",
    "react": "16.4.0",
    "react-dom": "16.4.0"
  }
}

index.tsx

import { DetailsList } from 'office-ui-fabric-react/lib/DetailsList';

tsconfig.json

{
  "compilerOptions": {
    "isolatedModules": true
  },
  "include": [ "index.tsx" ]
}

cmd

> npm run build

Bug Report

• __Package version(s)__: 6.61.0
• __Browser and OS versions__: Windows 10

Priorities and help requested:

Are you willing to submit a PR to fix? Yes

Requested priority: Normal

Products/sites affected: Internal build, workaroundable for now

Describe the issue:

We're using TypeScript and bundling our product with webpack. The webpack build transpiles modules in isolation. To pick up potential issues with this, we should have isolatedModules turned on in tsconfig.json.

office-ui-fabric-react blocks us from doing so, since a variety of imports cause the error:

node_modules/office-ui-fabric-react/lib/components/List/List.types.d.ts:5:27 - error TS1209: Ambient const enums are not allowed when the '--isolatedModules' flag is provided.

This occurs in a number of places - Importing DetailsList is just one example. It also seems the const in these files is not actually necessary, the enum is still present in all the built JS just as it would with the standard enum.

Actual behavior:

Build fails with isolatedModules turned on, due to a number of const enums defined in the package's type declarations.

Expected behavior:

Should be able to import office-ui-fabric-react and build successfully with isolatedModules turned on.

[andyrooger]

This similar or a duplicate of #2796 but since it has never had any activity and was then closed, I raised this with more information.

[kenotron]

I have a repro. But I want to know the use case for directly using isolatedModules. I've tested webpack + ts-loader in transpileOnly, and that seems to do okay with this setup. ts-jest also seems okay since we use it internally.

If we take this flag out, folks that use 'tsc' to build before webpack bundle step will see a bundle size increase. In fact, we recommend transpileOnly mode to be turned OFF for production builds and benefit from these const enum inlining optimizations.

[andyrooger]

@kenotron I see your point about consumers being able to inline these enums. I also feel like this is more a problem with TypeScript's const enum design than this project. The project will build and run fine right now, as you say, but the problem is that the decision to use const enum forces consumers to turn off safety for the rest of their projects.

Does the issue closure mean the discussion is over? If not, below are our particular use cases for this.

Use case for transpileOnly

In our case we were using webpack + ts-loader and a dev build would take upwards of ten or 15 minutes. Prod builds were a little slower. That's not an acceptable build time for us in either case.

Following the ts-loader docs, we set transpileOnly and added fork-ts-checker-webpack-plugin to keep type checking and it dropped to a little above 30 seconds.

Use case for isolatedModules

The build with transpileOnly succeeds, but we were getting runtime errors from it (not from this package). Turns out we had const enum in some declaration files. They used to be inlined, but stopped after we turned on transpileOnly, and the d.ts files obviously don't generate anything to import.

The type checker passed since it ran across all files at once, with no idea about transpileOnly. This is exactly the kind of error that isolatedModules is for. Turning that on means TypeScript should warn us about these potential errors at compile time.

We have fixed those particular errors now we've found them, but I'm not going to see any other potential issues (not necessarily const enum related) because office-ui-fabric-react blocks us from turning isolatedModules on and finding out.

ts-jest

I haven't used this but it looks like they're aware of the problem that const enum causes for transpileOnly code. office-ui-fabric-react seems to avoid runtime issues by essentially making them non-const internally with preserveConstEnums. This isn't something that will work for anyone consuming the package externally.

[andyrooger]

I've been trying to come up with some kind of workaround for now to allow us to use isolatedModules without any changes to this project.

1. Use a webpack plugin/loader to strip out const from all the enums in this package as they're loaded
   
   
   
   • Very dirty
   
   
   • Don't think the .d.ts files run through loaders though I have yet to confirm
   
   
2. Import ts directly from office-ui-fabric-react
   
   
   
   • Think const enum is only an issue in declaration files
   
   
   • npm package seems to just have .d.ts and .js
   
   

If anyone has any ideas for a better workaround, I'd appreciate it.

[kenotron]

I understand and appreciate the concern. I myself wrote the example for ts-loader here:
https://github.com/TypeStrong/ts-loader/tree/master/examples/fast-incremental-builds

I can see that you are running into many of the same problems as users of ts-loader. Now, I would encourage you to consider perhaps inlining const enums for production builds via transpileOnly turned OFF. Meanwhile, keep the preserveConstEnum somehow with incremental builds.

On Fabric side, change const enum back to enum should take a little bit of deprecation planning. I don't feel like we've explore that just yet. It might be that we switch ourselves into string union types instead. So, we'll want to address this, but probably via a refactoring towards a different data type altogether.

[andyrooger]

Thanks for the responses @kenotron . I've tested my two workarounds above, unfortunately without success.

I've also tested turning on preserveConstEnums but it is only helpful for code we're compiling ourselves, not for precompiled npm libraries with declarations. With your suggestions above we'll have a very slow prod build that should work ok, and a dev build with unpredictable runtime errors that don't occur in prod.

I think for now I'd rather take the faster builds for both, and so deal with the same set of potential runtime errors in both. Our built code is pretty large, and the bytes saved from enum inlining don't make a noticeable dent.

we'll want to address this, but probably via a refactoring towards a different data type altogether.

Is there anything I can do to contribute towards this? And is there anywhere this would be tracked other than this closed issue?

[kenotron]

I've opened a feature request in favor of getting rid of the const enum at least in OUFR package. Let's discuss a strategy there. #6386