Skip to content

Foundation: Remove Object.assign usage to fix IE11 issues #6089

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JasonGore merged 7 commits into from
Aug 28, 2018
Merged

Foundation: Remove Object.assign usage to fix IE11 issues #6089

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cd2c3d2
Remove Object.assign usage in Foundation to fix IE11 issues.
JasonGore Aug 27, 2018
4119795
Change file.
JasonGore Aug 27, 2018
e5b854b
Tweak naming consistency.
JasonGore Aug 27, 2018
091e5ff
Merge props consistently.
JasonGore Aug 27, 2018
5261be8
Revert customizations mixin to view props.
JasonGore Aug 27, 2018
d66cb8f
Use tslib's __assign for statics.
Aug 28, 2018
8344baf
Consolidate __assign usage.
JasonGore Aug 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions common/changes/@uifabric/foundation/jg-remove-object-assign_2018-08-27-15-56.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "@uifabric/foundation",
"comment": "Remove Object.assign usage to fix IE11 issues.",
"type": "patch"
}
],
"packageName": "@uifabric/foundation",
"email": "jagore@microsoft.com"
}
68 changes: 47 additions & 21 deletions packages/foundation/src/createComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ export interface IStylingProviders<TViewProps, TStyleSet, TProcessedStyleSet, TC
mergeStyleSets: (...styles: (Partial<TStyleSet> | undefined)[]) => TProcessedStyleSet;
getCustomizations: (scope: string, context: TContext) => IStyleableComponent<TViewProps, TStyleSet, TTheme>;
// TODO: remove any if possible
// tslint:disable-next-line:no-any
CustomizableContextTypes: any;
}

Expand Down Expand Up @@ -144,20 +143,21 @@ export function createComponent<
// The approach here is to allow state components to provide only the props they care about, automatically
// merging user props and processed props together. This ensures all props are passed properly to view,
// including children and styles.
// TODO: Should 'rest' props from customizations pass onto view? They are not currently.
// (items like theme seem like they shouldn't)
const propStyles = processedProps.styles || userProps.styles;
const themedProps: TProcessedProps = Object.assign({}, rest, userProps, processedProps);
const viewProps: IViewComponentProps<TProcessedProps, TProcessedStyleSet> = Object.assign(
{},
processedProps,
userProps,
{
const styleProps: TProcessedProps = { ...rest, ...(processedProps as any), ...(userProps as any) };
const viewProps: IViewComponentProps<TProcessedProps, TProcessedStyleSet> = {
...(processedProps as any),
...(userProps as any),
...{
classNames: providers.mergeStyleSets(
_evaluateStyle(themedProps, options.styles),
_evaluateStyle(themedProps, contextStyles),
_evaluateStyle(themedProps, propStyles)
_evaluateStyle(styleProps, options.styles),
_evaluateStyle(styleProps, contextStyles),
_evaluateStyle(styleProps, propStyles)
)
}
);
};

// TODO: consider rendering view as JSX component with display name in debug mode to aid in debugging
return options.view(viewProps);
Expand All @@ -169,7 +169,7 @@ export function createComponent<
result.contextTypes = providers.CustomizableContextTypes;
result.displayName = options.displayName;

Object.assign(result, options.statics);
_assignStatics(result, options.statics);

// Later versions of TypeSript should allow us to merge objects in a type safe way and avoid this cast.
return result as React.StatelessComponent<TComponentProps> & TStatics;
Expand Down Expand Up @@ -200,15 +200,20 @@ export function createStatelessComponent<
const { styles: contextStyles, ...rest } = settings;

const content = (processedProps: TProcessedProps) => {
// TODO: Should 'rest' props from customizations pass onto view? They are not currently.
// (items like theme seem like they shouldn't)
const { styles: propStyles } = processedProps;
const themedProps: TProcessedProps = Object.assign({}, rest, processedProps);
const viewProps: IViewComponentProps<TProcessedProps, TProcessedStyleSet> = Object.assign({}, processedProps, {
classNames: providers.mergeStyleSets(
_evaluateStyle(themedProps, options.styles),
_evaluateStyle(themedProps, contextStyles),
_evaluateStyle(themedProps, propStyles)
)
});
const styleProps: TProcessedProps = { ...rest, ...(processedProps as any) };
const viewProps: IViewComponentProps<TProcessedProps, TProcessedStyleSet> = {
...(processedProps as any),
...{
classNames: providers.mergeStyleSets(
_evaluateStyle(styleProps, options.styles),
_evaluateStyle(styleProps, contextStyles),
_evaluateStyle(styleProps, propStyles)
)
}
};

// TODO: consider rendering view as JSX component with display name in debug mode to aid in debugging
return options.view(viewProps);
Expand All @@ -219,12 +224,33 @@ export function createStatelessComponent<

result.contextTypes = providers.CustomizableContextTypes;
result.displayName = options.displayName;
Object.assign(result, options.statics);

_assignStatics(result, options.statics);

@cliffkoh cliffkoh Aug 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be easier to just do

Object.keys(option.statics).forEach(key => result[key] = option.statics[key]);

Instead of calling _assignStatics below which as far as I can tell is only used here.

@JasonGore JasonGore Aug 27, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't forEach not supported in IE11? This is a simplified version of assign in our own utilities package as I didn't want to add it as a dependency to Foundation.

@JasonGore JasonGore Aug 27, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Also there are 2 uses.)

@cliffkoh cliffkoh Aug 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is supposed in IE 9. Same for Object.keys...

@JasonGore JasonGore Aug 27, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer just to use our proven implementation here.

@JasonGore JasonGore Aug 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at __assign I'm hesitant to use it as it is documented as an internal helper. I spoke with David and we're having reservations using forEach as well. I think I'll leave it as-is for now.

I'm not sure where as lightweight as possible came from as a goal (I certainly never documented it anywhere as other than being light on dependencies) but certainly one of the more worthwhile goals I have is to combine the two separate createComponent functions as one.

@cliffkoh cliffkoh Aug 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JavaScript bundle size is ALWAYS a concern as it directly impacts page performance. The only case where we do not care is if the code never ends up being loaded and run in browser, such as in the case of build tools like rush, jest, etc.

W.r.t. your comment on __assign, you do realize that all the object spreads you've utilized to avoid calling object.assign directly are calling __assign under the hood? It makes no sense to avoid using __assign when you've all these other code that still ends up calling __assign. I would be fine however if you are really adverse to having a dependency on tslib (why make it an explicit dependency then of Foundation?) if you remove all usages of object spreads and have everything call your assign helper, so as to avoid 2 unnecessary copies of assign.

That's not ideal however - I think it makes more sense to just use __assign

@JasonGore JasonGore Aug 28, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course bundle size is always a concern, but it's also weighed with other considerations such as engineering time, design simplicity, reliability, time to market, etc. Everything's a balancing act, one consideration doesn't completely outweigh all of the others. In this case I decided to take a proven utility function from Utilities and make use of it here for the purposes of reliability. I felt that was worth the 5 lines of code.

I do also realize how tslib helpers are used, but again, it's an implementation detail under the hood. Our code doesn't use it directly anywhere and I think it's conceivable TS and tslib helpers could change API internally in lockstep together.

All implications aside, if nobody else has any great concern of its use, I will change the PR to use __assign.

@kenotron kenotron Aug 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa wait, don't take internal implementation of tslib as a dep here. That's subject to changes - typescript + tslib could change the internal naming on us and we'd have to update here. I prefer seeing a for loop here since it is foundation and we don't want take in "util" as a dep here. A simple function would do.

@JasonGore JasonGore Aug 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it just goes to show there is no "one right" solution for many engineering problems. Either I move back to using the for loop with a hasOwnProperty check (in a function to avoid duplication as I had originally) or I keep tslib usage in. I'm ok with either method if someone is willing to approve either one.


// Later versions of TypeSript should allow us to merge objects in a type safe way and avoid this cast.
return result as React.StatelessComponent<TComponentProps> & TStatics;
}

/**
* Basic Object.assign helper for applying statics to target.
* @param target Target object to merge into.
* @param statics Statics object that will be mixed into target.
* @returns Resulting merged target.
*/
function _assignStatics(target: any, statics: any): any {

@cliffkoh cliffkoh Aug 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why the 3 anys are not just objects?

@JasonGore JasonGore Aug 27, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function in Utilities can handle undefined args, as can this function.

@cliffkoh cliffkoh Aug 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so statics?: object or statics: object | undefined, instead of any?

@JasonGore JasonGore Aug 27, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's much of a difference either way.

target = target || {};

if (statics) {
for (const propName in statics) {
if (statics.hasOwnProperty(propName)) {
target[propName] = statics[propName];
}
}
}

return target;
}

/**
* Evaluate styles based on type to return consistent TStyleSet.
*/
Expand Down
3 changes: 2 additions & 1 deletion packages/foundation/tslint.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"office-ui-fabric-react-tslint"
],
"rules": {
"deprecation": false
"deprecation": false,
"no-any": false
}
}