Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

chore: Normalize custom types #2252

Merged
merged 10 commits into from
Jan 29, 2020
Merged

chore: Normalize custom types #2252

merged 10 commits into from
Jan 29, 2020

Conversation

ecraig12345
Copy link
Member

@ecraig12345 ecraig12345 commented Jan 16, 2020

Standardize patterns around custom types (with the goal of making it easier to add or modify types or debug issues):

Remove the typeRoots config from tsconfig.common.json and only set typeRoots in individual packages that need custom types. And remove places that include-ed the main types directory, since I think having it both include-ed and used as a typeRoot may confuse the compiler and/or editors.

Make the types directory follow the standard structure for a type root (<package name>/index.d.ts).

Add dependencies on @types/webpack-env and remove things from the custom global types which properly come from there.

Move the global types file to docs since that's the only place it's used.

Wrap screener-runner types in a module, and only import it from the projects that need it. For now it still exposes types globally, but eventually all the files referencing those types should be updated to use explicit imports.

Remove most custom types:

  • add deps on @types/classnames
  • add deps on @types/simulant which I wrote 😄 (and move main simulant dep to appropriate places)
  • remove anchor-js types since they weren't doing anything, and ts-ignore that import

Remove react from tsconfig types lists since it shouldn't be included ambiently.

Move docs, e2e, and perf tsconfigs to their respective projects instead of having them in build (same thing will be done with webpack configs later).

Add types to webpack configs.

Notes on using package names in tsconfig extends paths

In theory we should be able to do "extends": "@fluentui/internal-tooling/tsconfig.common.json" instead of "../../build/tsconfig.common.json". However, this can cause multiple problems...

  • If typeRoots or baseUrl is specified in the shared config, it will get resolved relative to node_modules/@fluentui/internal-tooling/tsconfig.common.json (because TS doesn't use realpaths when resolving configs). This is bad because it makes a lot of types and modules get resolved incorrectly.
  • Even after removing typeRoots and baseUrl from the shared config, I ran into some other issues (I think with module resolution)...unfortunately I don't remember the details, and it may be solvable, but I decided to stick with relative paths for now because there are too many other things to deal with.

I think the lack of realpath resolution is a bug or at least a desirable enhancement, so I'm going to file an issue with TS about that. (Naturally I'm having trouble getting an isolated repro...)

@DustyTheBot
Copy link
Collaborator

DustyTheBot commented Jan 29, 2020

Warnings
⚠️ There are no updates provided to CHANGELOG. Ensure there are no publicly visible changes introduced by this PR.
⚠️ Package (or peer) dependencies changed. Make sure you have approval before merging!

Changed dependencies are detected.

Changed dependencies in build/package.json

package before after
@types/webpack-env - ^1.14.1

Perf comparison

Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.57 0.37 1.54:1 2000 1134
🔧 Button.Fluent 1.23 0.23 5.35:1 1000 1229
🔧 Checkbox.Fluent 1.42 0.27 5.26:1 1000 1416
🔧 Dialog.Fluent 0.4 0.21 1.9:1 5000 2023
🔧 Dropdown.Fluent 3.69 0.37 9.97:1 1000 3691
🔧 Icon.Fluent 0.27 0.04 6.75:1 5000 1368
🔧 Image.Fluent 0.12 0.1 1.2:1 5000 616
🔧 Slider.Fluent 2.27 0.36 6.31:1 1000 2269
🦄 Text.Fluent 0.07 0.17 0.41:1 5000 373
🦄 Tooltip.Fluent 0.4 19.51 0.02:1 5000 1976

🔧 Needs work     🎯 On target     🦄 Amazing

Generated by 🚫 dangerJS

@ecraig12345
Copy link
Member Author

@layershifter @kenotron This appears to be done (build succeeded)! Please take a look when you get a chance.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants