Foundation: Remove Object.assign usage to fix IE11 issues

[JasonGore]

Pull request checklist

• Addresses an existing issue: Fixes #0000
• Include a change request file using $ npm run change

Description of changes

Remove Object.assign usage in Foundation to fix IE11 issues.

Microsoft Reviewers: Open in CodeFlow

[cliffkoh]

Any reason why the 3 anys are not just objects?

[JasonGore]

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

[cliffkoh]

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

[JasonGore]

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

[cliffkoh]

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]

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]

(Also there are 2 uses.)

[cliffkoh]

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

[JasonGore]

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

[JasonGore]

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]

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]

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]

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]

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.