Skip to content
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

fix: support different window frames #11

Closed
wants to merge 1 commit into from
Closed

fix: support different window frames #11

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,8 +360,8 @@ export function parseFromProgram(
return t.instanceOfNode(typeName);
}
case 'Element': {
// Nextjs: Element isn't defined on the server
return t.instanceOfNode("typeof Element === 'undefined' ? Object : Element");
// Nextjs: Element isn't defined on the server nor is stable between window frames.
return t.instanceOfNode('Object');
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that instanceOf is not suitable for Elements. However at the point we might as well is PropTypes.object or check against Node.nodeType. But this requires a custom validator.

I think a custom validator is better suited for this one (using #10)

Copy link
Author

@oliviertassinari oliviertassinari Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If I understand it correctly, with #10, we can do:

  /**
   * A node, component instance, or function that returns either.
   * The `container` will have the portal children appended to it.
   * By default, it uses the body of the top-level document object,
   * so it's simply `document.body` most of the time.
   */
  container: chainPropTypes(
    PropTypes.oneOfType([
      PropTypes.func,
      PropTypes.instanceOf(React.Component),
      PropTypes.instanceOf(typeof Element === 'undefined' ? Object : Element),
    ]),
    (props) => {
      if (props.container) {
        const resolvedContainer = getContainer(props.container);
        const containerWindow = ownerWindow(resolvedContainer);

        if (resolvedContainer != null && !(resolvedContainer instanceof containerWindow.Element)) {
          return new Error(
            [
              'Material-UI: the `container` prop provided to the component is invalid.',
              `It should be an Element instance but it's \`${resolvedContainer}\` instead.`,
            ].join('\n'),
          );
        }
      }
    },
  ),
detail 
diff --git a/docs/src/modules/components/MarkdownDocs.js b/docs/src/modules/components/MarkdownDocs.js
index 37a99a7f2..71464cb3e 100644
--- a/docs/src/modules/components/MarkdownDocs.js
+++ b/docs/src/modules/components/MarkdownDocs.js
@@ -117,6 +117,11 @@ function MarkdownDocs(props) {
   const prevPage = pageList[currentPageNum - 1];
   const nextPage = pageList[currentPageNum + 1];

+  React.useEffect(() => {
+    const container = document.querySelector('.description');
+    container.classList.add('ad');
+  }, []);
+
   return (
     <AppFrame>
       <Head
@@ -124,13 +129,7 @@ function MarkdownDocs(props) {
         description={headers.description || getDescription(markdownDocs.markdown)}
       />
       {disableAd ? null : (
-        <Portal
-          container={() => {
-            const container = document.querySelector('.description');
-            container.classList.add('ad');
-            return container;
-          }}
-        >
+        <Portal container={() => document.querySelector('.description')}>
           <Ad />
         </Portal>
       )}
diff --git a/packages/material-ui/src/Portal/Portal.js b/packages/material-ui/src/Portal/Portal.js
index 9d38da191..1d50de59c 100644
--- a/packages/material-ui/src/Portal/Portal.js
+++ b/packages/material-ui/src/Portal/Portal.js
@@ -1,14 +1,15 @@
 import * as React from 'react';
 import * as ReactDOM from 'react-dom';
 import PropTypes from 'prop-types';
-import { exactProp } from '@material-ui/utils';
+import { exactProp, chainPropTypes } from '@material-ui/utils';
 import setRef from '../utils/setRef';
+import ownerWindow from '../utils/ownerWindow';
 import useForkRef from '../utils/useForkRef';

 function getContainer(container) {
   container = typeof container === 'function' ? container() : container;
   // #StrictMode ready
-  return ReactDOM.findDOMNode(container);
+  return container;
 }

 const useEnhancedEffect = typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect;
@@ -72,11 +73,28 @@ Portal.propTypes = {
    * By default, it uses the body of the top-level document object,
    * so it's simply `document.body` most of the time.
    */
-  container: PropTypes.oneOfType([
-    PropTypes.func,
-    PropTypes.instanceOf(React.Component),
-    PropTypes.instanceOf(typeof Element === 'undefined' ? Object : Element),
-  ]),
+  container: chainPropTypes(
+    PropTypes.oneOfType([
+      PropTypes.func,
+      PropTypes.instanceOf(React.Component),
+      PropTypes.instanceOf(typeof Element === 'undefined' ? Object : Element),
+    ]),
+    (props) => {
+      if (props.container) {
+        const resolvedContainer = getContainer(props.container);
+        const containerWindow = ownerWindow(resolvedContainer);
+
+        if (resolvedContainer != null && !(resolvedContainer instanceof containerWindow.Element)) {
+          return new Error(
+            [
+              'Material-UI: the `container` prop provided to the component is invalid.',
+              `It should be an Element instance but it's \`${resolvedContainer}\` instead.`,
+            ].join('\n'),
+          );
+        }
+      }
+    },
+  ),
   /**
    * Disable the portal behavior.
    * The children stay within it's parent DOM hierarchy.

However, I don't think that it's enough/necessary:

  1. React will do the validation for us "Error: Target element is not a DOM node".
  2. We will still get the generation of PropTypes.instanceOf(typeof Element === 'undefined' ? Object : Element).

PropTypes.object

This sounds like a great proposal. What about we move forward with it? cc @merceyz.

Copy link
Contributor

Choose a reason for hiding this comment

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

Almost. We don't need instanceOf check at all as far as I can tell. Checking props.container.nodeType should be enough. To be honest I think most use cases should use nodeType. instanceOf is only useful if you want to make sure that an element comes from the same or different realm.

}
}
}
Expand Down