fix: support different window frames #11
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oliviertassinari
changed the title
oliviertassinari
changed the title
eps1lon
reviewed
Mar 30, 2020
| // 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'); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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:
- React will do the validation for us "Error: Target element is not a DOM node".
- 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.
There was a problem hiding this comment.
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Does simplicity win? Closes #3