[IconMenu] Add missing default iconStyle

[pdf]

Otherwise IconButton explodes due to undefined iconStyle

[newoga]

Thanks @pdf! If we move forward with this, could you put this in the object in alphabetical order?

[newoga]

@oliviertassinari @alitaheri This seems like it's an issue since we moved to Object.assign. The iconStyle prop could be undefined and you can't call Object.assign with undefined as the first argument. I'm not sure if we should set it as a defaultProp or if there is a better alternative.

[pdf]

Yeah, this does feel to me like a band-aid, rather than a fix for the root cause.

An alternative is to always use Object.assign({}, ...)

Probably worth noting that IconButton will also explode if you pass it a null iconStyle.

[newoga]

An alternative is to always use Object.assign({}, ...)

I agree that's the pattern we've been used in the past for other components for style props that may be undefined. I haven't fully considered the implications of the defaultProp approach vs adding an empty object in the Object.assign call.

The previous mergeStyles method that this replaced always used an empty Object to merge styles into.

[mbrookes]

An empty object in mergeStyles may be preferable, otherwise the default in the docs will show as {}.

[alitaheri]

Also note that all styles that are coming from props MUST not be touched, otherwise, reused instances will be mutated and hard-to-track bugs will infest the code! so, everywhere using props, we must do Object.assign({}, ...).

[newoga]

I agree.

Thanks for raising this @pdf! Could you make that change defaultProp -> Object.assign({}, ...)?

[pdf]

Give me a day or two @newoga - I want to take a closer look at this, pretty sure the issue is more wide-spread.

[pdf]

I think I checked all the other usage of Object.assign, and this appears to be the only particularly dangerous usage.

[nathanmarks]

@newoga Change looks good 👍