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

removed unecessary fragment #4682

Closed

Conversation

AndyOGo
Copy link

@AndyOGo AndyOGo commented Jun 27, 2018

fixes #4681
improves #4031

@timneutkens
Copy link
Member

As this will most likely cause key errors I guess we should do something like:

const Fragment = React.Fragment || ({children}) => <div>{children}</div>

🤔

@AndyOGo
Copy link
Author

AndyOGo commented Jun 28, 2018

@timneutkens
Hmh, good point.
At least since React 16 components can render Arrays too, docs:
https://reactjs.org/docs/react-component.html#render

And in my local tests it works with Inferno 5.

Though I agree it should be tested with other React-like libs like Preact and so on too.
I could also imagine that returning a keyed Array of JSX could be better solution for cross React-like library support, similiar to:

Class Container extends Componenet {
  render () {
     const {children} = this.props

     if (Array.isArray(children) {
       return children.map((child, index) => child.key = index && child)
     }

     return children
   }
 }

@AndyOGo
Copy link
Author

AndyOGo commented Jun 28, 2018

Though I ask my self, will you ever render multiple children?

@timneutkens
Copy link
Member

Yeah, my main thinking was libraries that don't support React.Fragment + array return. Though you could easily polyfill React.Fragment in that case by doing something like React.Fragment = ({children}) => children

lib/app.js Outdated
if (Array.isArray(children)) {
return children.map((child, index) => {
// only set key if it's null or undefined
if (child.key == null) {
Copy link
Author

Choose a reason for hiding this comment

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

@timneutkens
We could also just use a false check, though values like 0 would match that too incorrectly.

return <>{children}</>

// make sure to support all React-like libraries, whether they support Arrays, React.Fragment or not
if (Array.isArray(children)) {
Copy link
Author

@AndyOGo AndyOGo Jun 28, 2018

Choose a reason for hiding this comment

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

@timneutkens
That does not work for multiple children.
Inferno throws:

rootNode is null
getReactStack@http://localhost:3000/_next/static/commons/main.js:14619:17
reconcileHotReplacement@http://localhost:3000/_next/static/commons/main.js:14948:48
renderReconciler@http://localhost:3000/_next/static/commons/main.js:14962:7
asyncReconciledRender@http://localhost:3000/_next/static/commons/main.js:14970:3
proxiedRender@http://localhost:3000/_next/static/commons/main.js:14259:5
updateClassComponent@http://localhost:3000/_next/static/commons/main.js:7999:24
patchComponent@http://localhost:3000/_next/static/commons/main.js:8043:13
patch@http://localhost:3000/_next/static/commons/main.js:7808:13
render@http://localhost:3000/_next/static/commons/main.js:8402:13
renderReactElement@http://localhost:3000/_next/static/commons/main.js:9245:5
_callee4$@http://localhost:3000/_next/static/commons/main.js:9212:15
tryCatch@http://localhost:3000/_next/static/commons/main.js:16237:37
invoke@http://localhost:3000/_next/static/commons/main.js:16471:22
defineIteratorMethods/</prototype[method]@http://localhost:3000/_next/static/commons/main.js:16289:16
step@http://localhost:3000/_next/static/commons/main.js:166:22
_next@http://localhost:3000/_next/static/commons/main.js:181:9
_asyncToGenerator/</<@http://localhost:3000/_next/static/commons/main.js:188:7
Promise@http://localhost:3000/_next/static/commons/main.js:3255:7
_asyncToGenerator/<@http://localhost:3000/_next/static/commons/main.js:161:12
doRender@http://localhost:3000/_next/static/commons/main.js:9154:10
_callee2$@http://localhost:3000/_next/static/commons/main.js:9060:20
tryCatch@http://localhost:3000/_next/static/commons/main.js:16237:37
invoke@http://localhost:3000/_next/static/commons/main.js:16471:22
defineIteratorMethods/</prototype[method]@http://localhost:3000/_next/static/commons/main.js:16289:16
step@http://localhost:3000/_next/static/commons/main.js:166:22
_next@http://localhost:3000/_next/static/commons/main.js:181:9
_asyncToGenerator/</<@http://localhost:3000/_next/static/commons/main.js:188:7
Promise@http://localhost:3000/_next/static/commons/main.js:3255:7
_asyncToGenerator/<@http://localhost:3000/_next/static/commons/main.js:161:12
render@http://localhost:3000/_next/static/commons/main.js:9032:10
_callee$/<@http://localhost:3000/_next/static/commons/main.js:9003:13
notify/<@http://localhost:3000/_next/static/commons/main.js:11509:16
./node_modules/core-js/library/modules/_collection.js/module.exports/</<@http://localhost:3000/_next/static/commons/main.js:1316:22
notify@http://localhost:3000/_next/static/commons/main.js:11508:7
_callee2$@http://localhost:3000/_next/static/commons/main.js:10963:17
tryCatch@http://localhost:3000/_next/static/commons/main.js:16237:37
invoke@http://localhost:3000/_next/static/commons/main.js:16471:22
defineIteratorMethods/</prototype[method]@http://localhost:3000/_next/static/commons/main.js:16289:16
step@http://localhost:3000/_next/static/commons/main.js:166:22
_next@http://localhost:3000/_next/static/commons/main.js:181:9
run@http://localhost:3000/_next/static/commons/main.js:3153:22
notify/<@http://localhost:3000/_next/static/commons/main.js:3170:30
flush@http://localhost:3000/_next/static/commons/main.js:1960:9

and:

Children is an array
Warning: the 'url' property is deprecated. https://err.sh/next.js/url-deprecated
{ Error: Inferno Error: renderToString() received an object that's not a valid VNode, you should stringify it first. Object: "[{"childFlags":1,"children":null,"className":null,"dom":null,"flags":8,"isValidated":false,"key":0,"parentVNode":null,"props":{"url":{"query":{},"pathname":"/","asPath":"/"},"__source":{"fileName":"/Users/andy/scale-unlimited/webapp_v2/pages/_app.js","lineNumber":31}},"ref":null},{"childFlags":2,"children":{"childFlags":1,"children":"Hello world","className":null,"dom":null,"flags":16,"isValidated":false,"key":null,"parentVNode":null,"props":null,"ref":null,"type":null},"className":null,"dom":null,"flags":1,"isValidated":false,"key":1,"parentVNode":null,"props":{"__source":{"fileName":"/Users/andy/scale-unlimited/webapp_v2/pages/_app.js","lineNumber":32},"children":"Hello world"},"ref":null,"type":"div"}]".
    at throwError (/Users/andy/scale-unlimited/webapp_v2/node_modules/inferno-server/dist/index.cjs.js:39:11)
    at renderVNodeToString (/Users/andy/scale-unlimited/webapp_v2/node_modules/inferno-server/dist/index.cjs.js:297:17)
    at renderVNodeToString (/Users/andy/scale-unlimited/webapp_v2/node_modules/inferno-server/dist/index.cjs.js:195:20)
    at renderVNodeToString (/Users/andy/scale-unlimited/webapp_v2/node_modules/inferno-server/dist/index.cjs.js:195:20)
    at renderToString (/Users/andy/scale-unlimited/webapp_v2/node_modules/inferno-server/dist/index.cjs.js:308:12)
    at renderPage (/Users/andy/scale-unlimited/webapp_v2/node_modules/next/dist/server/render.js:277:26)
    at Function.getInitialProps (/Users/andy/scale-unlimited/webapp_v2/node_modules/next/dist/server/document.js:67:25)
    at _callee$ (/Users/andy/scale-unlimited/webapp_v2/node_modules/next/dist/lib/utils.js:111:30)
    at tryCatch (/Users/andy/scale-unlimited/webapp_v2/node_modules/regenerator-runtime/runtime.js:62:40)
    at GeneratorFunctionPrototype.invoke [as _invoke] (/Users/andy/scale-unlimited/webapp_v2/node_modules/regenerator-runtime/runtime.js:296:22)
    at GeneratorFunctionPrototype.prototype.(anonymous function) [as next] (/Users/andy/scale-unlimited/webapp_v2/node_modules/regenerator-runtime/runtime.js:114:21)
    at step (/Users/andy/scale-unlimited/webapp_v2/node_modules/@babel/runtime/helpers/asyncToGenerator.js:12:30)
    at _next (/Users/andy/scale-unlimited/webapp_v2/node_modules/@babel/runtime/helpers/asyncToGenerator.js:27:9)
    at /Users/andy/scale-unlimited/webapp_v2/node_modules/@babel/runtime/helpers/asyncToGenerator.js:34:7
    at Promise.F (/Users/andy/scale-unlimited/webapp_v2/node_modules/core-js/library/modules/_export.js:36:28)
    at /Users/andy/scale-unlimited/webapp_v2/node_modules/@babel/runtime/helpers/asyncToGenerator.js:7:12 sourceMapsApplied: true }


// make sure to support all React-like libraries, whether they support Arrays, React.Fragment or not
if (Array.isArray(children)) {
return <div>{children.map((child, index) => {
Copy link
Author

Choose a reason for hiding this comment

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

@timneutkens
Fixed above error by wrapping it within a <div> as root node

@AndyOGo
Copy link
Author

AndyOGo commented Jun 28, 2018

@timneutkens
I have at least working code now, but it could be more readable.

First I would like to assure that my understanding is correct.
The Container is supposed to be able to render single and multiple children right?

Regarding your suggestion I agree that patching React.Fragment if it's not defined is the cleanest approach. So I would say we have two cases to patch:

  • Single chlildren - this is easy, since it can just be returned, without any wrapping root node, like a <div>
  • Multiple children - this is rather complex, caused it needs proper keys at each child and needs a wrapping root node

May it's also overkill to differentiate between single and multiple children 🤔

@AndyOGo
Copy link
Author

AndyOGo commented Jun 28, 2018

There seems to be also a React.Fragment polyfill package:
https://www.npmjs.com/package/react-dot-fragment

@HaNdTriX
Copy link
Contributor

This issue has been resolved in: #4766

@HaNdTriX HaNdTriX closed this Aug 26, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Usage of React.Fragment in Container causes Inferno incompatability
3 participants