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.

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

Constrain the container type of createPortal #24496

Merged
merged 1 commit into from
May 5, 2022

Conversation

sebmarkbage
Copy link
Collaborator

We already constrained the type of createRoot (can't take document) and hydrateRoot (can't take shadow roots aka fragments).

We already constrained the type of createRoot (can't take document) and hydrateRoot (can't take fragments).
@@ -107,7 +107,7 @@ setBatchingImplementation(

function createPortal(
children: ReactNodeList,
container: Container,
container: Element | DocumentFragment,
key: ?string = null,
): React$Portal {
if (!isValidContainer(container)) {
Copy link
Collaborator Author

@sebmarkbage sebmarkbage May 5, 2022

Choose a reason for hiding this comment

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

We have this helper function but I think it doesn't make sense. We should test these independently since they're not all the same. This one should error on document for example.

However, should we also make it a DEV only warning so we don't need the code?

@sizebot
Copy link

sizebot commented May 5, 2022

Comparing: 0dc4e66...544a1b2

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 131.52 kB 131.52 kB = 42.10 kB 42.10 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 136.79 kB 136.79 kB = 43.68 kB 43.68 kB
facebook-www/ReactDOM-prod.classic.js = 441.02 kB 441.02 kB = 80.44 kB 80.44 kB
facebook-www/ReactDOM-prod.modern.js = 426.28 kB 426.28 kB = 78.25 kB 78.25 kB
facebook-www/ReactDOMForked-prod.classic.js = 441.02 kB 441.02 kB = 80.44 kB 80.44 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 544a1b2

@sebmarkbage sebmarkbage merged commit 024a727 into facebook:main May 5, 2022
@sebmarkbage
Copy link
Collaborator Author

@eps1lon I noticed that the TypeScript one is limited to Element but I think we technically support shadow roots too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants