createMuiTheme overrides not working since 4.9.6

[fbaeta]

createMuiTheme(a, b) overrides not working since 4.9.6

Since #20095 & #20100, the isPlainObject has been modified.

material-ui/packages/mui-utils/src/deepmerge.ts

Lines 1 to 3 in b935d3e

	export function isPlainObject(item: unknown): item is Record<keyof any, unknown> {
	return item !== null && typeof item === 'object' && item.constructor === Object;
	}

Testing constructorproperty is not multi-frame proof.
This makes deepmerge do not merge incoming properties from a given source when source is an Object created in another js context like another document iframe.

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The generated jss style does not conatin overrides, so desired style is not applied to components.

Expected Behavior 🤔

The generated jss style must contain overrides in order to have the desired styles applied to components.

Tech	Version
Material-UI	v4.1.11
React	16.14
Browser	all
TypeScript	no
etc.

[oliviertassinari]

I'm not sure we want to support this multi-frame case with the theme. How did you encounter this situation?

[fbaeta]

#23740 (comment)

https://github.com/jonschlinkert/is-plain-object might also be a source of inspiration. My main concern is about adding bundle size for edge case almost nobody needs. You can modify the theme outside of the createMuiTheme() method.

Before all, thanks for your fast reply!

I understand that edge cases do not justify increasing bundle size 👍.

I've created a PR just to show the case, the goal was not really o be merged ;) (the isPlainObject code is borrowed from lodash).
I wanted to point out that:

• Using constructor to test if it's a plain object is not the more resilient technique and should be reconsidered. If you don't like lodash implementation, as you said, you might find other techniques. This change was introduced here [Tooltip] - PopperProps broken starting from 4.9.5 #20095, so if in v5 you are planning to remove support of this popper version, you sould reconsider (again) to not use constructor technique.
• The isPlainObject is poorly tested.

In my current case, I CAN NOT upgrade material-ui up to 4.9.5 for now.

[eps1lon]

I'm not sure we want to support this multi-frame case. How did you encounter this situation?

We support it everywhere else. Why change it in this particular instance? That sounds way to confusing for library users unless you are familiar with the implementation of the library which defeats the purpose of using a library.

https://github.com/jonschlinkert/is-plain-object/blob/master/is-plain-object.js

@fbaeta Could you open a PR that replaces internal usage with this library instead?

It's apparent that this kind of functionality needs to be checked in a dedicated package. Current implementation shows a type of hubris that you usually find in people not familiar with the JS ecosystem: "just write your own. how hard can it be?". Well, we had two bugs now in code we wrote that we could've easily imported instead. That would have saved us time. Instead of sinking more time into the problem we should be able to recognize sunk-cost fallacy.

[oliviertassinari]

Why change it in this particular instance?

@eps1lon I'm asking because it doesn't seem to be something we fundamentally prevent from supporting. As far as I know, It's not blocking. Developers can bypass the deepmerge method, by bypassing the createMuiTheme() method. Hence why I'm eager to have a reproduction to better understand the use case.

We know that multi-frame support is rarely used but we still support it. I suspect this specific case is unique and nonblocking. A drop in the bucket most developers would rather have us ignore (not pay the cost for it). Not every bug deserved to be fixed. I think that it's a big factor in the "write your own" popularity, it allows different tradeoffs (but also come with ignoring problems you don't know you have).

[oliviertassinari]

@fbaeta Feel free to provide a reproduction. If there are no workarounds, we should probably consider a solution in Material-UI. However, you are mentioning the theme creator helper that can be bypassed, so we would very likely be better off ignoring this problem. We can also reconsider if more developers face this issue.

[fbaeta]

@fbaeta Feel free to provide a reproduction. If there are no workarounds, we should probably consider a solution in Material-UI. However, you are mentioning the theme creator helper that can be bypassed, so we would very likely be better off ignoring this problem. We can also reconsider if more developers face this issue.

I will give you a more isolated case of the use case that we are facing in some weeks.

You mention that theme creator can be bypassed. How? The theme creator is a feature of MUI, we are happy with it and we want to continue to use it.

Nevertheless,

1. Checking constrcutor is not a bullet proof javascript technique to detect if an object is a plain object
2. JSS is compatible with multiple iframes so MUI should too.

[oliviertassinari]

will give you a more isolated case of the use case that we are facing in some weeks.

@fbaeta Thanks! Do you have a specific use case for providing an element from a different window inside the theme?

JSS is compatible with multiple iframes so MUI should too.

MUI is, you can bypass createMuiTheme().

[Janpot]

We've hit this issue in Toolpad. See mui/toolpad#2514. We have a workaround, but I believe it would be better if MUI core provided a more defensive isPlainObject function.

[oliviertassinari]

Based on sindresorhus/is-plain-obj#11, this could fix it:

diff --git a/packages/mui-utils/src/deepmerge.test.ts b/packages/mui-utils/src/deepmerge.test.ts
index d0aceef896..641672c613 100644
--- a/packages/mui-utils/src/deepmerge.test.ts
+++ b/packages/mui-utils/src/deepmerge.test.ts
@@ -1,3 +1,4 @@
+import { runInNewContext } from 'vm';
 import { expect } from 'chai';
 import deepmerge from './deepmerge';

@@ -60,4 +61,13 @@ describe('deepmerge', () => {
     expect(result).to.deep.equal({ foo: { baz: 'new test' } });
     expect(foo).to.deep.equal({ foo: { baz: 'test' } });
   });
+
+  it('should deep clone with object of different realm', () => {
+    const foo = { foo: { baz: 'test' } };
+    const bar = runInNewContext('({})');
+
+    const result = deepmerge(bar, foo);
+
+    expect(result).to.deep.equal({ foo: { baz: 'test' } });
+  });
 });
diff --git a/packages/mui-utils/src/deepmerge.ts b/packages/mui-utils/src/deepmerge.ts
index 1d0c9a44b8..883f01e417 100644
--- a/packages/mui-utils/src/deepmerge.ts
+++ b/packages/mui-utils/src/deepmerge.ts
@@ -1,5 +1,11 @@
 export function isPlainObject(item: unknown): item is Record<keyof any, unknown> {
-  return item !== null && typeof item === 'object' && item.constructor === Object;
+  // Basic check
+  if (item == null || typeof item !== 'object') {
+    return false;
+  }
+
+  const prototype = Object.getPrototypeOf(item);
+  return prototype === null || Object.getPrototypeOf(prototype) === null;
 }

 export interface DeepmergeOptions {

https://stackoverflow.com/questions/49832187/how-to-understand-js-realms explains a bit why this could be valuable. Why not.