react-theme: support custom theme creation API

[khmakoto]

Spinning from comment thread in #20651.

We should look at how to support a self-explanatory API for consumers to create a custom theme.

We could do that by modifying the signature of mergeThemes to allow for this type of functionality:

export function mergeThemes<TTheme extends Theme = Theme, TPartialTheme extends PartialTheme = PartialTheme>(a: TTheme | undefined, b: TPartialTheme | undefined): TTheme & TPartialTheme {
   ...
}

And then we could do:

import { mergeThemes, webLightTheme } from '@fluentui/react-theme';

const customTokens = {...};
const customTheme = mergeTheme(webLightTheme, customTokens);
// easily accessible shape inferred from source of truth - actual implementation
type CustomTheme = typeof customTheme;

[Hotell]

I've been looking at the existing mergeThemes function to play with the idea, but I'd like to get some reasoning regarding its API before we move forward.

The API feels bit "strange" as user can pass:

• undefined as 1st argument mergeTheme(undefined, {...})
• 2nd argument is always required, can be undefined as well
  • Error mergeTheme({...})
  • OK mergeTheme({...}, undefined)

What API I'd suggest:

• ❌ doesn't make sense mergeTheme(undefined, extendedTheme)
• ❌ doesn't make sense mergeTheme(extendedTheme, undefined)
• ✅ all good mergeTheme(coreTheme, extendedTheme)

Why?

it should be consumer responsibility to check for existence of particular variables that he want's to pass to our API. Having our current api act as identity function when undefined are used feels obscure and hides the true purpose of this.

WDYT?

[varholak-peter]

Agreed, I am not a fan of optional first params. Do we simply want to swap the order of parameters or define the function in a different way?

[Hotell]

Agreed, I am not a fan of optional first params. Do we simply want to swap the order of parameters or define the function in a different way?

as in my proposal I'd have both required only - should be responsibility of consumer to check if arguments that he want's to pass are undefined

[khmakoto]

Yeah, I think it would make sense to remove the undefined from the parameters.

CCing @layershifter and @miroslavstastny to get their input on that.

What do you think of the rest of the proposal? Do you think it makes sense to extend mergeThemes in this way?

P.S. I think you do from your comments but wanted to make sure I'm not just reflecting my own assumptions 😅

[layershifter]

It makes sense to change mergeThemes() to have first param as always defined. But few things should be updated then:

Update useFluentProvider.ts

fluentui/packages/react-provider/src/components/FluentProvider/useFluentProvider.ts

Line 30 in d704e0f

const mergedTheme = mergeThemes(parentTheme, theme);

-const mergedTheme = mergeThemes(parentTheme, theme); 
+const mergedTheme = mergeThemes(parentTheme || {} as Theme, theme); 

(not sure if type cast will be necessary)

Update ThemeContext.ts

It's reasonable to cleanup typings and match implementation. Not sure why we went with casting null to Theme initially.

fluentui/packages/react-shared-contexts/src/ThemeContext/ThemeContext.ts

Lines 4 to 6 in d704e0f

	export const ThemeContext = React.createContext((null as unknown) as ThemeContextValue);
	export function useTheme(): ThemeContextValue {

-export const ThemeContext = React.createContext((null as unknown) as ThemeContextValue);
+export const ThemeContext = React.createContext<ThemeContextValue | undefined>(undefined);

-export function useTheme(): ThemeContextValue {
+export function useTheme(): ThemeContextValue | undefined {

Other option: update ThemeContextValue itself to include undefined.

[Hotell]

Not sure why we went with casting null to Theme initially.

I guess because of this "Note: passing undefined as a Provider value does not cause consuming components to use defaultValue." - meaning if explicitly set to null, consumers will get he actual null.

---

I'd go with your suggested api change for ThemeContext and the hook.
Current state is providing misleading information to consumers, that they will get an existing value(themeContext dictionary).

With that, this needs to change as well (yes assertion is needed):

// $ExpectType Theme
const mergedTheme = mergeThemes(parentTheme || {} as Theme, theme); 

• this is wrong as based on logic it can be Theme or {}, but because the cast it will be always Theme
• {} is a footgun in typescript (bottom type)
• with previous statements, I'd rather have our API's sound/explicit thus the mergedTheme should be Theme | Partial<Theme> | null
  • that will need to be accommodated in other related apis starting with FluentProviderState shape

Question based on previous: can there be a situation when both theme and parentTheme are null/undefined (is this allowed behavior) ?

---

Removing mergeThemes completely

As we talked yesterday the mergeThemes can be probably removed completely as it is just merging 2 objects - which is a vanilla js operation - thus we could exposed less API/cognitive overhead to consumers.

You wanna create custom theme ? just merge base and your custom one

const customTheme = {...lightTheme,...customThemeTokens}

[Hotell]

based on our discussion on todays sync we agreed to proceed with removing mergeThemes completely from react-theme api. Additional guidance should be provided to our docs.