Adding componentRef support, BaseComponent warnings, and initial Customizer component. #1389

dzearing · 2017-04-01T18:41:10Z

Addresses issue: #1356

This change is a subset of a much larger change, but this includes the important updates which are more general:

In components which are wrapped in a decorator or HOC, the "ref" ends up pointing to the wrong component, hiding all public methods (like focus().) In order to access the actual "ref", I'm adding support for resolving componentRef to the BaseComponent. This lets us pass through the callback to an inner component in decorator/HOC patterns. If you use BaseComponent as a base class, it will by default resolve it for you; you simply expose it in your Props definition. If you would like to avoid this being done because the inner component is doing it, you can set the protected _shouldUpdateComponentRef bool to false which will bypass resolution at that component layer.
Added the Customizer component and customizable decorator for injection. These helpers will enable us to inject new default props into components without requiring every individual component to be subclassed.
Deprecation maps should no longer be passed into the BaseComponent constructor. Instead you can use BaseComponent protected helpers _warnDeprecations and _warnMutuallyExclusive which will console.warn when deprecated things are being used.

…omizer implementation, adding warning helpers.

dzearing · 2017-04-01T18:47:37Z

packages/utilities/src/BaseComponent.ts


-    return (results && results.length > 1) ? results[1] : '';
+    return this.__className;


memoizing the classname as an optimization.

dzearing · 2017-04-01T18:48:36Z

packages/utilities/src/BaseComponent.ts

+   * never changes.
+   */
+  protected _onRenderNull(): JSX.Element | null {
+    return null;


Just as an fyi; I use this quite a bit in my other Button PR where i would like to return a function that renders nothing, but don't want to recreate it over and over (to avoid immutability issues)

Should we move this to a const outside of a class?

These new warns are awesome. I want to make sure to communicate this to all developers. Considering a weekly React update to gather all of these new changes in a newsletter type form.

dzearing · 2017-04-01T18:49:34Z

packages/utilities/src/BaseComponent.ts

@@ -153,7 +235,7 @@ function _makeSafe(obj: BaseComponent<any, any>, prototype: Object, methodName:
        if (prototypeMethod) {
          retVal = prototypeMethod.apply(this, arguments);
        }
-        if (classMethod) {
+        if (classMethod !== prototypeMethod) {


This is an important bug fix; Previously, it would call the prototypeMethod twice if the subclass doesn't implement it.

dzearing · 2017-04-01T18:50:10Z

packages/utilities/src/Customizer.tsx

+import { BaseComponent } from './BaseComponent';
+import { assign } from './object';
+
+export interface ISettings {


I'd like to check this in as an initial implementation, but will add examples and refine this in a separate PR.

Note that this isn't being exported yet until we use it.

cschleiden · 2017-04-03T15:42:35Z

packages/utilities/src/BaseComponent.ts

@@ -44,6 +61,16 @@ export class BaseComponent<P, S> extends React.Component<P, S> {
    ]);
  }

+  /** When the component will receive props, make sure the componentRef is updated. */
+  public componentWillReceiveProps(newProps?: any, newContext?: any) {


Or IBaseProps<P>?

cschleiden · 2017-04-03T15:44:08Z

packages/utilities/src/BaseComponent.ts

+   * never changes.
+   */
+  protected _onRenderNull(): JSX.Element | null {
+    return null;


Should we move this to a const outside of a class?

cschleiden · 2017-04-03T15:56:15Z

packages/utilities/src/warn.ts

@@ -0,0 +1,63 @@
+let _warningCallback = _warn;
+
+export interface IStringMap {


Can we improve typing here? Something like this:

type ISettingsMap<T> = { [P in keyof T]?: string; }; function foo<T>(object: T, deprecations: ISettingsMap<T>) { for (let x in deprecations) { alert(deprecations[x]); } } //////// interface IX { foo: number; bar: string; }; var x: IX = { foo: 42, bar: "test" }; foo<IX>(x, { "foo": "abc" });

(see http://www.typescriptlang.org/play/index.html#src=type%20ISettingsMap%3CT%3E%20%3D%20%7B%0D%0A%20%20%20%20%5BP%20in%20keyof%20T%5D%3F%3A%20string%3B%0D%0A%7D%3B%0D%0A%0D%0Afunction%20foo%3CT%3E(object%3A%20T%2C%20deprecations%3A%20ISettingsMap%3CT%3E)%20%7B%0D%0A%20%20%20%20for%20(let%20x%20in%20deprecations)%20%7B%0D%0A%20%20%20%20%20%20%20%20alert(deprecations%5Bx%5D)%3B%0D%0A%20%20%20%20%7D%0D%0A%7D%0D%0A%0D%0A%2F%2F%2F%2F%2F%2F%2F%2F%0D%0A%0D%0Ainterface%20IX%20%7B%0D%0A%20%20%20%20foo%3A%20number%3B%0D%0A%20%20%20%20bar%3A%20string%3B%0D%0A%7D%3B%0D%0A%0D%0Avar%20x%3A%20IX%20%3D%20%7B%0D%0A%20%20%20%20foo%3A%2042%2C%0D%0A%20%20%20%20bar%3A%20%22test%22%0D%0A%7D%3B%0D%0A%0D%0Afoo%3CIX%3E(x%2C%20%7B%0D%0A%20%20%20%20%22foo%22%3A%20%22abc%22%0D%0A%7D)%3B)

thx for the comments, will try

Good news! I did this, and it caught some bugs where I misplaced some warnings! Yay typings!

…c-react into general-updates

…tly works, even if the component doesn't have a public interface. Adding placeholder public interfaces, which exposes a number of inconsistencies across the components ( all of the focusable components should have focus methods, for example)

cschleiden · 2017-04-03T20:25:43Z

packages/office-ui-fabric-react/src/components/Checkbox/Checkbox.Props.ts

+   */
+  componentRef?: (component: ICheckbox) => void;
+
+  /**


As discussed offline, I think it would be good if we could move this to an interface

interface IComponentRef<TComponent> { componentRef?: (component: TComponent) => void; }

and then add it to the component's props by extending multiple interfaces? Or by & it in BaseComponent? Shouldn't be a blocker for this change, but going forward it would be nice to centralize these shared props to make refactoring etc. easier.

cschleiden · 2017-04-03T20:59:05Z

(Left it as a comment on an outdated, commit, repeating here for visibility)

As discussed offline, I think it would be good if we could move this (componentRef declaration) to an interface

interface IComponentRef<TComponent> {
  componentRef?: (component: TComponent)  => void;
}

and then add it to the component's props by extending multiple interfaces? Or by & it in BaseComponent? Shouldn't be a blocker for this change, but going forward it would be nice to centralize these shared props to make refactoring etc. easier.

…oving the deprecation map from BaseComponent, as it is conflicting with React's context when it is passed into the constructor.

cschleiden · 2017-04-04T01:15:35Z

packages/utilities/src/BaseComponent.ts

@@ -38,17 +38,12 @@ export class BaseComponent<P extends IBaseProps, S> extends React.Component<P, S
   * @param {Object} deprecatedProps The map of deprecated prop names to new names, where the key is the old name and the


update comment?

cschleiden · 2017-04-04T01:15:43Z

packages/utilities/src/BaseComponent.ts

@@ -38,17 +38,12 @@ export class BaseComponent<P extends IBaseProps, S> extends React.Component<P, S
   * @param {Object} deprecatedProps The map of deprecated prop names to new names, where the key is the old name and the
   * value is the new name. If a prop is removed rather than renamed, leave the value undefined.
   */
-  constructor(props?: P, deprecationMap?: ISettingsMap<P>) {
+  constructor(props?: P) {


take context parameter and pass along?

dzearing added 3 commits April 1, 2017 11:15

Updates to BaseComponent to support componentRef, adding initial Cust…

917ab34

…omizer implementation, adding warning helpers.

Updating components to use warning helpers.

568c6f7

Updates to use componentRef

2bac382

msftclas added the cla-not-required label Apr 1, 2017

dzearing commented Apr 1, 2017

View reviewed changes

cschleiden reviewed Apr 3, 2017

View reviewed changes

dzearing added 4 commits April 3, 2017 13:04

Updating typings pr feedback, upgrading typescript.

eadb066

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

0d6dacf

…c-react into general-updates

Updating test.

70bc13a

cschleiden approved these changes Apr 3, 2017

View reviewed changes

dzearing added 4 commits April 3, 2017 16:23

lint

aaf6220

more lint

2c02959

Merge branch 'master' into general-updates

85aafdf

Adding change file. Updating all components to use BaseComponent. Rem…

9ce8a1b

…oving the deprecation map from BaseComponent, as it is conflicting with React's context when it is passed into the constructor.

cschleiden reviewed Apr 4, 2017

View reviewed changes

Updating comment, passing along context.

7a5c103

dzearing merged commit 086833b into microsoft:master Apr 4, 2017

dzearing deleted the general-updates branch April 4, 2017 04:19

micahgodbolt mentioned this pull request Apr 6, 2017

How to access public interfaces for components (replacement for ref) #1356

Closed

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding componentRef support, BaseComponent warnings, and initial Customizer component. #1389

Adding componentRef support, BaseComponent warnings, and initial Customizer component. #1389

dzearing commented Apr 1, 2017 •

edited

Loading

dzearing Apr 1, 2017

dzearing Apr 1, 2017

cschleiden Apr 3, 2017

micahgodbolt Apr 3, 2017

dzearing Apr 1, 2017

dzearing Apr 1, 2017 •

edited

Loading

cschleiden Apr 3, 2017

cschleiden Apr 3, 2017

cschleiden Apr 3, 2017

cschleiden Apr 3, 2017

dzearing Apr 3, 2017

dzearing Apr 3, 2017

cschleiden Apr 3, 2017

cschleiden commented Apr 3, 2017

cschleiden Apr 4, 2017

cschleiden Apr 4, 2017


		return (results && results.length > 1) ? results[1] : '';
		return this.__className;

		@@ -0,0 +1,63 @@
		let _warningCallback = _warn;

		export interface IStringMap {

		@@ -38,17 +38,12 @@ export class BaseComponent<P extends IBaseProps, S> extends React.Component<P, S
		* @param {Object} deprecatedProps The map of deprecated prop names to new names, where the key is the old name and the

Adding componentRef support, BaseComponent warnings, and initial Customizer component. #1389

Adding componentRef support, BaseComponent warnings, and initial Customizer component. #1389

Conversation

dzearing commented Apr 1, 2017 • edited Loading

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

dzearing Apr 1, 2017 • edited Loading

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

cschleiden commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzearing commented Apr 1, 2017 •

edited

Loading

dzearing Apr 1, 2017 •

edited

Loading