chore: move out Fela to a separate package #13602

layershifter · 2020-06-15T09:28:11Z

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ yarn change

Description of changes

BREAKING CHANGES

This PR changes design of renderer injection to Fluent UI Northstar. The renderer prop was removed from Provider component, now we accept createRender factory via a React context.

Before

<Provider renderer={renderer} />

After

<RendererContext.Provider value={() => renderer}>
  <Provider  />
</ProviderContext.Provider>

(!) There are no breaking changes if you never used the renderer prop on Provider.

Details ❔

The previous design with renderer was created before we started to support multiple documents and blocks us from introducing custom renderers. That's why we decided to go with a factory that will allow us to create renderers on demand.

This PR also updates Renderer type and unifies its usage across Fluent Northstar code. This will allow us to introduce different renderers expect Fela.

Out of scope ✂️

This PR will not remove the dependency from react-fela and its ThemeContext which we are using everywhere. This thing should be tackled separately.

layershifter · 2020-06-15T09:34:30Z

packages/fluentui/react-bindings/src/renderer/RendererContext.ts

+import { CreateRenderer } from '@fluentui/styles';
+import * as React from 'react';
+
+export const RendererContext = React.createContext<CreateRenderer>(createFelaRenderer);


We decided to have only the context now without any additional custom hooks/providers.

layershifter · 2020-06-15T09:35:22Z

packages/fluentui/react-bindings/src/styles/resolveStyles.ts

-  const felaParam: RendererParam = {
-    theme: { direction },
+  const rendererParam: RendererParam = {
+    direction,


For a unified RendererParam it will be strange to pass theme.direction.

layershifter · 2020-06-15T09:36:50Z

packages/fluentui/react-bindings/src/styles/resolveStyles.ts

@@ -41,7 +43,6 @@ const stylesCache = new WeakMap<ThemePrepared, Record<string, ICSSInJSStyle>>();
 const resolveStyles = (
  options: ResolveStylesOptions,
  resolvedVariables: ComponentVariablesObject,
-  renderStylesInput?: (styles: ICSSInJSStyle) => string,


This was used only for tests actually, but as we already using renderRule from options.renderer I decided to remove it to simplify the logic.

layershifter · 2020-06-15T09:37:58Z

packages/fluentui/react-bindings/src/styles/types.ts

-export type RendererRenderRule = (rule: () => ICSSInJSStyle, param: RendererParam) => string;
-export type Renderer = Omit<FelaRenderer, 'renderRule'> & {
-  renderRule: RendererRenderRule;
-};


Move out to @fluentui/styles, not sure that it's a right place, but:

it's definitely not a part of react-bindings

it should be available for both renderer packages

Otherwise we can have a circular dependency:

@fluentui/react-northstar-emotion-renderer: error TS6202: Project references may not form a circular graph. Cycle detected: /mnt/work/1/s/packages/fluentui/react-northstar-emotion-renderer/tsconfig.json @fluentui/react-northstar-emotion-renderer: /mnt/work/1/s/packages/fluentui/react-bindings/tsconfig.json @fluentui/react-northstar-emotion-renderer: /mnt/work/1/s/packages/fluentui/react-northstar-fela-renderer/tsconfig.json

https://github.com/microsoft/fluentui/pull/13602/files#r440234899 ?

layershifter · 2020-06-15T09:38:23Z

packages/fluentui/react-bindings/src/styles/types.ts

  theme?: ThemeInput;
 };

-export type StylesContextValue<R = Renderer> = {
+export type StylesContextValue = {


Types were simplified as now we have a single Renderer type.

layershifter · 2020-06-15T09:41:55Z

packages/fluentui/react-northstar-fela-renderer/src/felaExpandCssShorthandsPlugin.ts

@@ -1,6 +1,8 @@
 import { ICSSInJSStyle } from '@fluentui/styles';
 import * as CSS from 'csstype';
+// @ts-ignore
 import { expandProperty } from 'inline-style-expand-shorthand';


This package doesn't have typings :(

layershifter · 2020-06-15T09:42:17Z

packages/fluentui/react-northstar-fela-renderer/src/felaPerformanceEnhancer.ts

@@ -5,7 +5,7 @@
 //

 import { ICSSInJSStyle } from '@fluentui/styles';
-// eslint-disable-next-line import/no-extraneous-dependencies
+// @ts-ignore
 import cssifyDeclaration from 'css-in-js-utils/lib/cssifyDeclaration';


No typings :(

layershifter · 2020-06-15T09:43:04Z

packages/fluentui/react-northstar/package.json

-    "fela-plugin-placeholder-prefixer": "^10.6.1",
-    "fela-plugin-rtl": "^10.6.1",
-    "fela-utils": "^10.6.1",
-    "inline-style-expand-shorthand": "^1.2.0",


These deps were moved to @fluentui/react-northstar-fela-renderer

layershifter · 2020-06-15T10:14:16Z

packages/fluentui/docs/src/components/VariableResolver/VariableResolver.tsx

-    </Provider>
-  );
+  return null;
+  // TODO: fix this feature


Variable resolver feature is now broken and should be fixed separately.

layershifter · 2020-06-15T10:15:50Z

packages/fluentui/react-northstar-fela-renderer/src/felaDisableAnimationsPlugin.ts

+ * Caution! Infinite recursion is possible in case if style object has links to self in the props
+ * tree.
+ */
+const felaDisableAnimationsPlugin = (


All plugins received a small refactor:

were simplified where options are not required

typings improved

Actually the main reason to do it: it's avoid fighting with typings.

layershifter · 2020-06-15T10:16:51Z

packages/fluentui/react-northstar-fela-renderer/test/felaSanitizeCssPlugin-test.ts

-      propertyWithInvalidValue: 'rgba(',
-    };
-
-    expect(sanitize(style, { skip: ['propertyWithInvalidValue'] })).toEqual(style);


This feature was removed to simplify the returned signature. Was never used anywhere.

packages/fluentui/react-northstar/src/utils/mergeProviderContexts.ts

size-auditor · 2020-06-15T11:00:09Z

Asset size changes

⚠️ Insufficient baseline data to detect size changes

Unable to find bundle size details for Baseline commit: eb0a36e

Possible causes

The baseline build eb0a36e is broken
The Size Auditor run for the baseline build eb0a36e was not triggered

Recommendations

Please merge your branch for this Pull request with the latest master build and commit your changes once again

packages/fluentui/react-bindings/src/styles/resolveStyles.ts

packages/fluentui/react-bindings/src/styles/types.ts

packages/fluentui/react-northstar-fela-renderer/package.json

packages/fluentui/react-northstar/test/specs/utils/felaRenderer-test.tsx

packages/fluentui/CHANGELOG.md

…om/microsoft/fluentui into chore/move-out-fela

miroslavstastny · 2020-06-16T08:07:12Z

packages/fluentui/react-bindings/tsconfig.json

@@ -25,6 +25,9 @@
    },
    {
      "path": "../react-component-ref"
+    },
+    {
+      "path": "../react-northstar-fela-renderer"


Just a question - do we have/can we add any way to enforce project references to be up-to-date?

I don't know about it, but if there will be a missing reference it will throw.

packages/fluentui/react-northstar-fela-renderer/test/felaInvokeKeyframesPlugin-test.ts

miroslavstastny · 2020-06-16T09:41:20Z

packages/fluentui/react-northstar-fela-renderer/src/felaFocusVisibleEnhancer.ts

@@ -27,7 +12,7 @@ const felaFocusVisibleEnhancer = (renderer: Renderer) => {
  renderer._emitChange = (change: RendererChange) => {
    if (change.type === RULE_TYPE && change.selector.indexOf(':focus-visible') !== -1) {
      // Fela uses objects by references, it's safe to override properties
-      change.pseudo = change.pseudo ? change.pseudo.replace(':focus-visible', ':focus') : undefined;
+      change.pseudo = change.pseudo ? change.pseudo.replace(':focus-visible', ':focus') : '';


Strict typings are better as it should be a string and will default to an empty string:

https://github.com/robinweser/fela/blob/97148419e7941dbd5867b35df6ac4de6c5ed3e55/packages/fela/src/createRenderer.js#L224
https://github.com/robinweser/fela/blob/97148419e7941dbd5867b35df6ac4de6c5ed3e55/packages/fela/src/createRenderer.js#L302-L309

...luentui/react-northstar/test/specs/utils/mergeProviderContexts/mergeProviderContexts-test.ts

…erContexts/mergeProviderContexts-test.ts Co-authored-by: Miroslav Stastny <mistastn@microsoft.com>

layershifter requested review from dvdzkwsk, dzearing, jurokapsiar, levithomason, miroslavstastny, mnajdova and pompomon as code owners June 15, 2020 09:28

msft-github-bot assigned jdhuntington Jun 15, 2020

layershifter force-pushed the chore/move-out-fela branch from a635658 to e703e01 Compare June 15, 2020 09:33

layershifter commented Jun 15, 2020

View reviewed changes

layershifter force-pushed the chore/move-out-fela branch from e703e01 to 88ea366 Compare June 15, 2020 09:41

layershifter commented Jun 15, 2020

View reviewed changes

layershifter force-pushed the chore/move-out-fela branch from 88ea366 to 9b85309 Compare June 15, 2020 09:47

layershifter added the Fluent UI react-northstar (v0) Work related to Fluent UI V0 label Jun 15, 2020

layershifter force-pushed the chore/move-out-fela branch from 9b85309 to b4945d0 Compare June 15, 2020 10:13

layershifter commented Jun 15, 2020

View reviewed changes

packages/fluentui/react-northstar/src/utils/mergeProviderContexts.ts Outdated Show resolved Hide resolved

layershifter force-pushed the chore/move-out-fela branch 4 times, most recently from 75ccc1c to 3c94456 Compare June 15, 2020 12:38

mnajdova reviewed Jun 15, 2020

View reviewed changes

packages/fluentui/react-bindings/src/styles/resolveStyles.ts Outdated Show resolved Hide resolved

address review comment

211c72c

mnajdova reviewed Jun 15, 2020

View reviewed changes

packages/fluentui/react-bindings/src/styles/types.ts Show resolved Hide resolved

layershifter added 2 commits June 15, 2020 17:08

move back dep

b5e2c10

remove duplicate import

46a08a3

mnajdova reviewed Jun 15, 2020

View reviewed changes

packages/fluentui/react-northstar-fela-renderer/package.json Outdated Show resolved Hide resolved

remove author field

74fe6bf

mnajdova reviewed Jun 15, 2020

View reviewed changes

packages/fluentui/react-northstar/test/specs/utils/felaRenderer-test.tsx Outdated Show resolved Hide resolved

layershifter added 4 commits June 15, 2020 17:49

fix resolver issue

09c9336

move out the test, avoid there components usage

84a5987

fix lint issue

d060fd0

fix ts issue

c555793

miroslavstastny reviewed Jun 16, 2020

View reviewed changes

packages/fluentui/CHANGELOG.md Outdated Show resolved Hide resolved

layershifter added 2 commits June 16, 2020 09:43

Merge branches 'chore/move-out-fela' and 'master' of https://github.c…

cd65dad

…om/microsoft/fluentui into chore/move-out-fela

update CHANGELOG.md

ec0438e

miroslavstastny reviewed Jun 16, 2020

View reviewed changes

mnajdova reviewed Jun 16, 2020

View reviewed changes

packages/fluentui/react-northstar-fela-renderer/test/felaInvokeKeyframesPlugin-test.ts Show resolved Hide resolved

rename to getRenderer

f706ffc

miroslavstastny reviewed Jun 16, 2020

View reviewed changes

mnajdova approved these changes Jun 16, 2020

View reviewed changes

miroslavstastny reviewed Jun 16, 2020

View reviewed changes

...luentui/react-northstar/test/specs/utils/mergeProviderContexts/mergeProviderContexts-test.ts Outdated Show resolved Hide resolved

Update packages/fluentui/react-northstar/test/specs/utils/mergeProvid…

0e20409

…erContexts/mergeProviderContexts-test.ts Co-authored-by: Miroslav Stastny <mistastn@microsoft.com>

miroslavstastny approved these changes Jun 16, 2020

View reviewed changes

layershifter mentioned this pull request Jun 16, 2020

chore: move out renderer typings from styles package #13616

Merged

2 tasks

layershifter merged commit d97ee4e into master Jun 16, 2020

layershifter deleted the chore/move-out-fela branch June 16, 2020 10:50

layershifter restored the chore/move-out-fela branch June 16, 2020 10:50

layershifter deleted the chore/move-out-fela branch June 16, 2020 10:50

layershifter mentioned this pull request Jun 23, 2020

docs: fix theme it feature #13740

Merged

2 tasks

layershifter mentioned this pull request Jul 8, 2020

chore: remove React from @fluentui/styles dependencies #13950

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: move out Fela to a separate package #13602

chore: move out Fela to a separate package #13602

layershifter commented Jun 15, 2020 •

edited

layershifter Jun 15, 2020 •

edited

layershifter Jun 15, 2020

layershifter Jun 15, 2020

layershifter Jun 15, 2020

layershifter Jun 15, 2020

mnajdova Jun 15, 2020

layershifter Jun 15, 2020

layershifter Jun 15, 2020

layershifter Jun 15, 2020

layershifter Jun 15, 2020

layershifter Jun 15, 2020

layershifter Jun 15, 2020

layershifter Jun 15, 2020

size-auditor bot commented Jun 15, 2020 •

edited

miroslavstastny Jun 16, 2020

layershifter Jun 16, 2020

miroslavstastny Jun 16, 2020

layershifter Jun 16, 2020

chore: move out Fela to a separate package #13602

chore: move out Fela to a separate package #13602

Conversation

layershifter commented Jun 15, 2020 • edited

Pull request checklist

Description of changes

BREAKING CHANGES

Before

After

Details ❔

Out of scope ✂️

layershifter Jun 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

size-auditor bot commented Jun 15, 2020 • edited

Asset size changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Jun 15, 2020 •

edited

layershifter Jun 15, 2020 •

edited

size-auditor bot commented Jun 15, 2020 •

edited