From b9aefca3895f12846c475ba6256c49e06243f6b5 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Mon, 4 Apr 2022 09:11:12 +0200 Subject: [PATCH 1/9] RFC: Global context registration for v9 This RFC proposes a solution for #21338 that involves registering one single context version per major in the global namespace to be consumed within Fluent. --- .../convergence/global-contex-registration.md | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 rfcs/react-components/convergence/global-contex-registration.md diff --git a/rfcs/react-components/convergence/global-contex-registration.md b/rfcs/react-components/convergence/global-contex-registration.md new file mode 100644 index 0000000000000..ab30f81100d3b --- /dev/null +++ b/rfcs/react-components/convergence/global-contex-registration.md @@ -0,0 +1,95 @@ +# RFC: Global context registration + +@ling1726 + +## Summary + +This RFC proposes that all Fluent UI contexts be registered in the global scope when created. Global scop means +`window` for browser environments or the `global` object for nodejs environments when using SSR. + +## Problem statement + +Many of our partners have complex dependency trees. The complexity in dependency management means that duplicate +packages can exist in `node_modules` folders. Although duplicate depedencies should generally be avoided, this still +happens quite often due to these factors: + +- Propagating dependency bumps downstream can be slow +- Engineering teams prioritize other work +- Incorrect version ranges declared + +Duplicated code generally is not a huge issue unless there are singletons that are duplicated. React contexts are +singletons. When duplicated: + +```tsx + + + + +React.useContext(ContextV1.2); +``` + +Scenarios like the above can be very difficult to debug. The value of `useContext` will always be the context +default value because it is not actually wrapped by its provider. The example above makes the reading simpler +by suffixing the version number in the name. In reality both contexts would actually share the same name even if +they are different. + +The issue has been created before in React's GitHub repository: + +- [facebook/react#13346](https://github.com/facebook/react/issues/13346) +- [reactjs/reactjs.org#1112](https://github.com/reactjs/reactjs.org/pull/1112) + +## Detailed Design or Proposal + +Prototypes can be found in the following repos for create-react-app and next.js SSR respectively: + +- https://github.com/ling1726/global-context +- https://github.com/ling1726/global-context-ssr + +The proposed solution involves a wrapper around `React.createContext` which also uses a user defined name, package name +and package major version as a key to create a symbol on the global object. + +When using this solution, the underlying javascript is not actually typed, so any backwards compatible context values +will work. However, if the context value change is breaking (e.g. removing a property, changing from string to object) +then the package containing the context should be major bumped. + +The solution will result in one single context that is used for each major version. + +[Symbol](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol) +is used as the key for global contexts, since symbol properties on objects are only visible through the +`Object.getOwnPropertySymbols` API, this will reduce the possibility that developers will access these global +contexts to abuse them. `Symbol` is quite an old APi that is supported from Chrome version 40 and Firefox version 36. + +Although not in the prototypes, it should be possible to create a global flag that will enable this behaviour in +Fluent, which means that this feature **should be completely opt-in**, and only intended for partners with complex +apps and dependency chains. + +### Pros and Cons + +#### Cons + +- Polluting the global namespace +- Custom magic when creating contexts in Fluent +- Generally using window/node globals is an antipattern + +#### Pros + +- One single major version context regardless of duplicate dependencies +- Not too different from `React.createContext` - we still use React contexts +- Global contexts are 'obfuscated' on window/node global with `Symbol` +- Functionality can be explicitly enabled + +## Discarded Solutions + +One of the solutions proposed in [facebook/react#13346](https://github.com/facebook/react/issues/13346), is to use +peer dependencies for the library so that apps are responsible for installing the final dependency once. + +Peer dependencies have been discarded because: + +- Mismatching peer deps only result in a warning (that tends to be ignored) +- Hard to determine version compatibility +- Sometimes apps want duplicate versions (e.g. during upgrades) +- No partner currently uses Fluent as a peer dependency -> lots of work to migrate + +## Open Issues + +[microsoft/fluentui#21338](https://github.com/microsoft/fluentui/issues/21338) From 60927e2baac5da393967c2638df05b02deb7a430 Mon Sep 17 00:00:00 2001 From: ling1726 Date: Wed, 6 Apr 2022 11:39:02 +0200 Subject: [PATCH 2/9] Update rfcs/react-components/convergence/global-contex-registration.md Co-authored-by: Martin Hochel --- rfcs/react-components/convergence/global-contex-registration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/react-components/convergence/global-contex-registration.md b/rfcs/react-components/convergence/global-contex-registration.md index ab30f81100d3b..335949f49c17a 100644 --- a/rfcs/react-components/convergence/global-contex-registration.md +++ b/rfcs/react-components/convergence/global-contex-registration.md @@ -4,7 +4,7 @@ ## Summary -This RFC proposes that all Fluent UI contexts be registered in the global scope when created. Global scop means +This RFC proposes that all Fluent UI contexts be registered in the global scope when created. Global scope means `window` for browser environments or the `global` object for nodejs environments when using SSR. ## Problem statement From b8254475a9e91ccc0634b022d6e3fe701cbe40a1 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 6 Apr 2022 17:11:23 +0200 Subject: [PATCH 3/9] suggestions --- .../convergence/global-contex-registration.md | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/rfcs/react-components/convergence/global-contex-registration.md b/rfcs/react-components/convergence/global-contex-registration.md index ab30f81100d3b..b3ad47d11e64e 100644 --- a/rfcs/react-components/convergence/global-contex-registration.md +++ b/rfcs/react-components/convergence/global-contex-registration.md @@ -45,6 +45,58 @@ Prototypes can be found in the following repos for create-react-app and next.js - https://github.com/ling1726/global-context - https://github.com/ling1726/global-context-ssr +```ts +import * as React from 'react'; +import { major } from 'semver'; + +type GlobalObject = typeof globalThis & Record>; +const isBrowser = typeof window !== 'undefined'; +const globalObject: GlobalObject = isBrowser ? window : global; + +// Identifier for the symbol, for easy idenfitifaction of symbols created by this util +// Useful for clearning global object during SSR reloads +const SYMBOL_NAMESPACE = 'global-context:'; + +// During SSR the global object persists with the server process +// Clean out the global object during server reload during development +if (!isBrowser && process.env.NODE_ENV !== 'production') { + const globalSymbols = Object.getOwnPropertySymbols(globalObject); + globalSymbols.forEach(sym => { + if (Symbol.keyFor(sym)?.startsWith(SYMBOL_NAMESPACE)) { + console.log('deleting', sym); + delete globalObject[sym]; + } + }); +} + +/** + * Wrapper around @see React.createContext that implements context registration + * in the globalThis object to avoid duplicate contexts. Contexts are keyed with + * a unique sybmol for the package name, version and name of the context. + * + * @see {@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol} + * + * @param defaultValue - @see React.createContext + * @param name - name of the context + * @param packageName - name of the npm package where the module is used + * @param packageVersion - version of the npm package where the module is used + * @returns @see React.createContext + */ +export const createContext = (defaultValue: T, name: string, packageName: string, packageVersion: string) => { + // Symbol guaranteed to be unique for the entire runtime + const sym = Symbol.for(`${SYMBOL_NAMESPACE}${packageName}/${name}/@${major(packageVersion)}`); + + // Objects keyed with symbols are not visible with console.log + // Object symbol properties can't be iterated with `for` or `Object.keys` + const globalSymbols = Object.getOwnPropertySymbols(globalObject); + if (!globalSymbols.includes(sym)) { + globalObject[sym] = React.createContext(defaultValue); + } + + return globalObject[sym] as React.Context; +}; +``` + The proposed solution involves a wrapper around `React.createContext` which also uses a user defined name, package name and package major version as a key to create a symbol on the global object. @@ -77,6 +129,7 @@ apps and dependency chains. - Not too different from `React.createContext` - we still use React contexts - Global contexts are 'obfuscated' on window/node global with `Symbol` - Functionality can be explicitly enabled +- Functionality can be applied with post processing so that we still use `React.createContext` internally ## Discarded Solutions From 0ddb7578fcaa55600e600d4c27c142ab50882912 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 4 May 2022 15:50:10 +0200 Subject: [PATCH 4/9] Add forwards/backwards compat --- .../convergence/global-contex-registration.md | 63 ++++++++++++++++++- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/rfcs/react-components/convergence/global-contex-registration.md b/rfcs/react-components/convergence/global-contex-registration.md index 1982c610dd684..4b11650840535 100644 --- a/rfcs/react-components/convergence/global-contex-registration.md +++ b/rfcs/react-components/convergence/global-contex-registration.md @@ -115,21 +115,78 @@ Although not in the prototypes, it should be possible to create a global flag th Fluent, which means that this feature **should be completely opt-in**, and only intended for partners with complex apps and dependency chains. -### Pros and Cons +### Forwards/Backwards compatibility for context default values -#### Cons +Backwards compatibility needs to be supported without a question because we need to follow semver. +However, since there is no guarantee which version of the context gets registered first in the global namespace, the default +values in our context will be unsafe by default. This introduces the constraint of forward comaptibility for our +context values. + +```tsx +// v1 context is registered first +const Contextv1 = React.createContext({ foo: 'xxx' }); +const Contextv11 = React.createContext({ foo: 'xxx', bar: 'yyyy' }); + +// v1.1 context is not registered and uses v1 context as proposed above +// ⚠️⚠️⚠️ bar is undefined but typings suggest it is defined +const { bar } = React.useContex(Contextv11); +``` + +In order to work around this problem this RFC proposes practices to follow in Fluent UI with regards to our +context values: + +- Never export contexts from Fluent UI directly +- Export context providers +- Context default values should always be undefined +- Export hook to access context +- Default values should be assigned in context hooks + +```tsx +interface FooContextValue { + foo: string; + bar: string; +} + +const FooContext = React.createContext>({}); + +export const FooContextProvider = FooContext.Provider; +export const useFooContext: FooContextValue = () => { + const ctx = React.useContext(FooContext); + + // We enforce all default react contexts to be empty + // if there are any properties we don't need to worry about default value + if (Object.keys(ctx).length) { + return ctx; + } + + const fooContextDefaultValue: FooContextValue = { + foo: 'xxx', + bar: 'yyy', + }; + return fooContextDefaultValue; +}; +``` + +We only care about the cases where the context value is an object since it context values that are primitives +cannot ever be extended as it would result in a breaking change. + +## Pros and Cons + +### Cons - Polluting the global namespace - Custom magic when creating contexts in Fluent - Generally using window/node globals is an antipattern +- Enforces stricter practices for using contexts -#### Pros +### Pros - One single major version context regardless of duplicate dependencies - Not too different from `React.createContext` - we still use React contexts - Global contexts are 'obfuscated' on window/node global with `Symbol` - Functionality can be explicitly enabled - Functionality can be applied with post processing so that we still use `React.createContext` internally +- Not exporting the actual context is good API encapsulation ## Discarded Solutions From 062e0b5583a3610cfbd03c79c3cd10121542b7c0 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 4 May 2022 16:14:24 +0200 Subject: [PATCH 5/9] feedback --- .../convergence/global-contex-registration.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/rfcs/react-components/convergence/global-contex-registration.md b/rfcs/react-components/convergence/global-contex-registration.md index 4b11650840535..a8de414cc66c2 100644 --- a/rfcs/react-components/convergence/global-contex-registration.md +++ b/rfcs/react-components/convergence/global-contex-registration.md @@ -119,7 +119,7 @@ apps and dependency chains. Backwards compatibility needs to be supported without a question because we need to follow semver. However, since there is no guarantee which version of the context gets registered first in the global namespace, the default -values in our context will be unsafe by default. This introduces the constraint of forward comaptibility for our +values in our context will be unsafe by default. This introduces the constraint of forward compatibility for our context values. ```tsx @@ -129,7 +129,7 @@ const Contextv11 = React.createContext({ foo: 'xxx', bar: 'yyyy' }); // v1.1 context is not registered and uses v1 context as proposed above // ⚠️⚠️⚠️ bar is undefined but typings suggest it is defined -const { bar } = React.useContex(Contextv11); +const { bar } = React.useContext(Contextv11); ``` In order to work around this problem this RFC proposes practices to follow in Fluent UI with regards to our @@ -178,6 +178,7 @@ cannot ever be extended as it would result in a breaking change. - Custom magic when creating contexts in Fluent - Generally using window/node globals is an antipattern - Enforces stricter practices for using contexts +- Bundle size increase for any context (+ 18.95 kB minified / + 5.224 kB gzipped) ### Pros From 438977acde73ad9fd370d839c70fa1a7f453553b Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 4 May 2022 16:17:56 +0200 Subject: [PATCH 6/9] bundle size --- .../react-components/convergence/global-contex-registration.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rfcs/react-components/convergence/global-contex-registration.md b/rfcs/react-components/convergence/global-contex-registration.md index a8de414cc66c2..3fcc31aaadd61 100644 --- a/rfcs/react-components/convergence/global-contex-registration.md +++ b/rfcs/react-components/convergence/global-contex-registration.md @@ -178,7 +178,8 @@ cannot ever be extended as it would result in a breaking change. - Custom magic when creating contexts in Fluent - Generally using window/node globals is an antipattern - Enforces stricter practices for using contexts -- Bundle size increase for any context (+ 18.95 kB minified / + 5.224 kB gzipped) +- Bundle size increase (+ 18.95 kB minified / + 5.224 kB gzipped) + - mainly caused by `semver` package ### Pros From 5c2e7e574697e35e56cdcf3f61a8a1769c577d5d Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 4 May 2022 16:25:56 +0200 Subject: [PATCH 7/9] discarded solution --- rfcs/react-components/convergence/global-contex-registration.md | 1 + 1 file changed, 1 insertion(+) diff --git a/rfcs/react-components/convergence/global-contex-registration.md b/rfcs/react-components/convergence/global-contex-registration.md index 3fcc31aaadd61..c07a99a96e85a 100644 --- a/rfcs/react-components/convergence/global-contex-registration.md +++ b/rfcs/react-components/convergence/global-contex-registration.md @@ -201,6 +201,7 @@ Peer dependencies have been discarded because: - Hard to determine version compatibility - Sometimes apps want duplicate versions (e.g. during upgrades) - No partner currently uses Fluent as a peer dependency -> lots of work to migrate +- Peer deps will only solve the problem, once all dependencies in the tree use Fluent as a peer dep ## Open Issues From 5dbe332c147b43269d6a3aacf6d834fa2d96b82a Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 25 May 2022 16:51:20 +0200 Subject: [PATCH 8/9] Update with code transform info --- .../convergence/global-contex-registration.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/rfcs/react-components/convergence/global-contex-registration.md b/rfcs/react-components/convergence/global-contex-registration.md index c07a99a96e85a..21e1897ba5684 100644 --- a/rfcs/react-components/convergence/global-contex-registration.md +++ b/rfcs/react-components/convergence/global-contex-registration.md @@ -45,6 +45,11 @@ Prototypes can be found in the following repos for create-react-app and next.js - https://github.com/ling1726/global-context - https://github.com/ling1726/global-context-ssr +A more detailed prototype has been created in the following repo, which also shows code transforms as a possible +solution to application + +- https://github.com/ling1726/global-context + ```ts import * as React from 'react'; import { major } from 'semver'; @@ -170,6 +175,18 @@ export const useFooContext: FooContextValue = () => { We only care about the cases where the context value is an object since it context values that are primitives cannot ever be extended as it would result in a breaking change. +### Application through code transforms + +Since the problem only occurs once `createContext` is invoked in the global file scope, it should be feasible +to apply the global context shim at build time for applications. The information needed to create a key for the +context (package name, version, context name) is all available at build time. + +This result would mean that there would need to be no direct code changes to Fluent UI. This solution would also +be explicitly opt-in for customers that might need a temporary quick solution while they are cleaning up their +dependency tree so that only one version of Fluent UI is in a bundle. + +> 💡 [A prototype code transform can be found here](https://github.com/bsunderhus/create-global-context-babel-transformer) + ## Pros and Cons ### Cons @@ -189,6 +206,7 @@ cannot ever be extended as it would result in a breaking change. - Functionality can be explicitly enabled - Functionality can be applied with post processing so that we still use `React.createContext` internally - Not exporting the actual context is good API encapsulation +- No code changes to Fluent UI ## Discarded Solutions From c04e7db892241bb2c34ef73fe9adb50f1a491742 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 25 May 2022 16:56:17 +0200 Subject: [PATCH 9/9] fix linke --- rfcs/react-components/convergence/global-contex-registration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/react-components/convergence/global-contex-registration.md b/rfcs/react-components/convergence/global-contex-registration.md index 21e1897ba5684..5a84570f10248 100644 --- a/rfcs/react-components/convergence/global-contex-registration.md +++ b/rfcs/react-components/convergence/global-contex-registration.md @@ -48,7 +48,7 @@ Prototypes can be found in the following repos for create-react-app and next.js A more detailed prototype has been created in the following repo, which also shows code transforms as a possible solution to application -- https://github.com/ling1726/global-context +- https://github.com/bsunderhus/create-global-context-babel-transformer ```ts import * as React from 'react';