[Bug]: Interfaces for FabricConfig are misaligned throughout Fluent.

brandonthomas commented 2 years ago

Library

React / v8 (@fluentui/react)

System Info

System:
    OS: Windows 10 10.0.22000
    CPU: (12) x64 Intel(R) Core(TM) i7-8700K CPU @ 3.70GHz
    Memory: 8.75 GB / 31.94 GB
  Browsers:
    Edge: Spartan (44.22000.120.0), Chromium (106.0.1370.34)
    Internet Explorer: 11.0.22000.120

NOTE: I'm on Win11 Pro not 10

Are you reporting Accessibility issue?

no

Reproduction

N/A - compiler error

Bug Description

Interfaces and uses of FabricConfig are not lined up properly. If a consumer imports font-icons-mdl2 they'll get the global defined within that file and this cannot be overridden (TS feature request: https://github.com/microsoft/TypeScript/issues/36146).

Actual Behavior

If font-icons-mdl2 is imported, the window.FabricConfig interface is set globally. Unfortunately, this then doesn't align with the expected interface that Stylesheet.ts uses. As a result teams downstream doing more complex things or attempting to pull this config are stuck just @ts-ignore'ing their own globals to get around the problem.

Global from Fluent:

https://github.com/microsoft/fluentui/blob/6112cd720c2a15635eae7175f22ec53510dd511c/packages/font-icons-mdl2/src/index.ts#L31-L57

Uses in Stylesheet and thus what consuming teams would need if they want to grab a config and align:

https://github.com/microsoft/fluentui/blob/a40470aff64a92bae7bb560bfd69192405001b9e/packages/merge-styles/src/Stylesheet.ts#L97-L103

Results in this error: TS2717: Subsequent property declarations must have the same type. Property 'FabricConfig' must be of type '{ fontBaseUrl?: string | undefined; iconBaseUrl?: string | undefined; } | undefined', but here has type '{ mergeStyles?: IStyleSheetConfig | undefined; } | undefined'.

Expected Behavior

FabricConfig uses IFabricConfig and all places within FluentUI React are aligned.

Logs

No response

Requested priority

Normal

Products/sites affected

MADS

Are you willing to submit a PR to fix?

yes

Validations

[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] The provided reproduction is a minimal reproducible example of the bug.

tudorpopams commented 1 year ago

@brandonthomas this might have a risk of being a breaking change. Can this be done in a backwards compatible way (ie a union interface with both the new and old ones)?

brandonthomas commented 1 year ago

@brandonthomas this might have a risk of being a breaking change. Can this be done in a backwards compatible way (ie a union interface with both the new and old ones)?

I'm for sure open to that solution as it seems better than the alternative. Provided there are no conflicts it would make sense.

brandonthomas commented 1 year ago

For context we wanted to fix this so we could grab the nonce and use it for injection. This isn't blocking since we used ts-ignore but it would definitely be nice to get type safety back.