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

Rendering fragments #501

Closed
darsain opened this issue Dec 2, 2016 · 47 comments
Closed

Rendering fragments #501

darsain opened this issue Dec 2, 2016 · 47 comments

Comments

@darsain
Copy link

darsain commented Dec 2, 2016

Or in other words, support for components to render multiple children:

const Component = () => [<Child1 />, <Child2 />];

It was said on slack it's a wanted feature with intention to be implement in future, so I'm creating this issue to have a place to track the progress.

Someone mentioned that it was already once in Inferno, but at the cost of performance, so it was dropped until a faster implementation is discovered. Was the performance hit really that bad? Because I'd gladly take it if it means the ability to render fragments. Inferno is one of the fastest frameworks out there, so it definitely doesn't lack the perf budget.

I consider this to be an essential feature to not have DOM polluted with useless wrapper elements which just get in the way of developing, maintaining, and styling components. And React already supports this in their Fiber rewrite, which is mostly done.

@thysultan
Copy link
Contributor

Mounting fragments is cheap, it's the moving/removing fragments that introduces complications since they can be seen as essentially a paired set. I'm interested in knowing how React-Fiber rewrite does this if they do and to what extent.

@leeoniya
Copy link

leeoniya commented Dec 2, 2016

having a similar convo over at domvm/domvm#106

@darsain
Copy link
Author

darsain commented Dec 2, 2016

@thysultan support for fragment rendering in React Fiber is mentioned here:

https://github.com/acdlite/react-fiber-architecture#child-and-sibling

Also I've heard it described in one talk, but can't recall enough about it to find it again :)

And if we are comparing different solutions, mithril (rewrite branch) also supports it.

Mithril also flattens all children by default (pretty useful when templating in hyperscript), so you can pass nested arrays and it gets resolved properly. Dunno if that is also the behavior in Inferno/React.

@trueadm
Copy link
Member

trueadm commented Dec 19, 2016

So this will likely going in for the future along with my refactor of normalization within Inferno. It won't be in for 1.0 due to the amount of edge cases it brings and things it breaks for now.

@trueadm trueadm self-assigned this Dec 19, 2016
@trueadm trueadm added this to the Future milestone Dec 19, 2016
@trueadm
Copy link
Member

trueadm commented Dec 23, 2016

We can add this to the post 1.0 roadmap. @Havunen @lukeed mind adding please?

@effulgentsia
Copy link

#628 is now closed. Should this issue be reopened now that React 16 is out and has this feature?

@Havunen
Copy link
Member

Havunen commented Oct 24, 2017

Yeah lets re-open, it will be there in next major release.

@Havunen Havunen reopened this Oct 24, 2017
@Havunen
Copy link
Member

Havunen commented Dec 18, 2017

This will come in v5

@rjarmstrong
Copy link

@Havunen I see you've been doing some work on this recently - does it look like it may be released soon?

@Havunen
Copy link
Member

Havunen commented Jan 7, 2018

Next big release will be v4, which does not include fragments. They were dropped out due to some bugs. Also fragments are in jsx plugin which is currently in beta. I think its better to wait babel / facebook to stabilize the feature and then we can implement it.

@SeanHayes
Copy link
Contributor

v5 is out, is this now supported?

@IgnusG
Copy link

IgnusG commented Apr 23, 2018

@SeanHayes V5 was in this case a major breaking change fix with esm modules. So this will most likely come with V6

@AndyOGo
Copy link

AndyOGo commented Jun 27, 2018

Unfortunately Next.js is using the short syntax of React.Fragment for it's Container's render method, which breaks usability of next.js with Inferno 😞

Related issue at next.js:
vercel/next.js#4031

@AndyOGo
Copy link

AndyOGo commented Jun 27, 2018

Here is the original feature discussion for fragments in jsx:
facebook/jsx#84

@AndyOGo
Copy link

AndyOGo commented Jun 27, 2018

This could also be solved by babel JSX plugin similar to:
https://github.com/jquense/babel-plugin-jsx-fragment

@kurdin
Copy link
Contributor

kurdin commented Jun 29, 2018

We need Fragment, more and more React components using it.

@sprlwrksAprilmintacpineda
Copy link

sprlwrksAprilmintacpineda commented Jul 24, 2018

So what happened? is there any progress about this feature? I'd love to know :-)

@Havunen
Copy link
Member

Havunen commented Jul 31, 2018

I`m in USA FL atm. This is currently first task in my todo list. I will continue it Aug 6th unless somebody wants to send PR :-)

@Havunen
Copy link
Member

Havunen commented Sep 29, 2018

This is now finally out for testing! Please see
https://github.com/infernojs/inferno/releases/tag/v6.0.0-rc.0

@simonjoom
Copy link

Hmm i tried the last one v6.0.0-rc.1 still Fragment is not working for me

@Havunen
Copy link
Member

Havunen commented Oct 4, 2018

@simonjoom do you use JSX? Did you remember to update babel-plugin-inferno to 6.0.0-5https://github.com/infernojs/babel-plugin-inferno/blob/dev/package.json

Are you getting any error message?

@simonjoom
Copy link

i ve got the latest inferno took from github and builded manually.
But yes i think i should to try with the babel you mention (i used only web pack aliases)

I created a repo merging gatsby with inferno, it's working nice)

https://github.com/simonjoom/gatsby-starter-inferno-master

@simonjoom
Copy link

simonjoom commented Oct 7, 2018

hello, i just tried without success with the branch dev as well.
Fragment is not working.

the plugin is well installed with webpack and babel config
for a tag <>blabla</>
i ve got this error
Inferno Error: createElement() name parameter cannot be undefined, null, false or true, It must be a string, class

So i tried with

import {Fragment} from 'React
<Fragment >blabla</Fragment>

I ve got "export 'Fragment' was not found in 'react' as well
i use ssr of gastby not sure it's related but..

Thanks

@trueadm
Copy link
Member

trueadm commented Oct 7, 2018

@simonjoom I believe you're meant to use Inferno.createFragment() if you're not intending to use the Babel transform (or maybe you're not using the latest version?). The base API is not a 1:1 of the React API, it's a different API that the Babel transform makes look similar.

@IgnusG
Copy link

IgnusG commented Oct 7, 2018

Hmm, did you configure the babel plugin correctly @simonjoom?
Check out if the fragment syntax is correct (reference here https://github.com/infernojs/babel-plugin-inferno/blob/6071ba7c24106ad4eb57f1d7ffbf8e8271f9b456/README.md#fragments)

And check if your configuration in .babelrc is correct (reference here https://github.com/infernojs/babel-plugin-inferno/blob/6071ba7c24106ad4eb57f1d7ffbf8e8271f9b456/README.md#how-to-use)

The blobs point to the latest release of that plugin 6.0.0-5

I can't find the babel plugin in your gatsby starter. If you do not intent to use it, you must write the fragments using what @trueadm suggested (check out the tests for the plugin's fragments implementation - basically what the plugin transforms your jsx code into infernojs/babel-plugin-inferno@6071ba7#diff-ce4097fa41097a3b63603226458faf55R309)

@Havunen
Copy link
Member

Havunen commented Oct 7, 2018

I think the issue was because new methods were not exported from inferno-compat. I will make new release

@Havunen
Copy link
Member

Havunen commented Oct 7, 2018

@simonjoom can you test inferno-6.0.0-rc.3 this was probably issue with Inferno compat pkg

@simonjoom
Copy link

simonjoom commented Oct 8, 2018

hello yes it's working great now:) thanks
just i saw a little bug if a fragment is around a single react node,

ok i know it's stupid to do
<><myComponent/></>
but it should to render <myComponent/> and not return null.

Maybe ping KyleAMathews from gatsby about the status of inferno, i think it's interesting for gatsby now.
look my thread on it:
gatsbyjs/gatsby#8858

@simonjoom
Copy link

simonjoom commented Oct 8, 2018

Hmm, did you configure the babel plugin correctly @simonjoom?
Check out if the fragment syntax is correct (reference here https://github.com/infernojs/babel-plugin-inferno/blob/6071ba7c24106ad4eb57f1d7ffbf8e8271f9b456/README.md#fragments)

And check if your configuration in .babelrc is correct (reference here https://github.com/infernojs/babel-plugin-inferno/blob/6071ba7c24106ad4eb57f1d7ffbf8e8271f9b456/README.md#how-to-use)

The blobs point to the latest release of that plugin 6.0.0-5

I can't find the babel plugin in your gatsby starter. If you do not intent to use it, you must write the fragments using what @trueadm suggested (check out the tests for the plugin's fragments implementation - basically what the plugin transforms your jsx code into infernojs/babel-plugin-inferno@6071ba7#diff-ce4097fa41097a3b63603226458faf55R309)

about the starter repo i did, i did not updated properly it, so well yes babel-plugin was not well configured when you wrote these lines.
I just did now on this commit
simonjoom/gatsby-starter-inferno-master@c03916b

@Havunen
Copy link
Member

Havunen commented Oct 8, 2018

@simonjoom if you are rendering component it needs to be written <><MyComponent/></> notice uppercase, otherwise its handled as vNode element ( div, span etc... ).

That use case should work unless there is bug somewhere.
For now; Please check test cases for documentation: https://github.com/infernojs/inferno/blob/master/packages/inferno/__tests__/fragments.spec.jsx

@simonjoom
Copy link

simonjoom commented Oct 8, 2018

yes sorry i wrote badly here. but on my app it's well written like:
<><MyComponent/></>

and it's return nothing in rendering so yes it's a bug from inferno

Nota if i add an other component or a node inside then MyComponent well render
<><div/><MyComponent/></>

@Havunen
Copy link
Member

Havunen commented Oct 8, 2018

@simonjoom Interesting, can you create steps to reproduce it somewhere? Maybe github repository? I need to be away for a while, but when I'm back I would like to have a look at that issue.

@Havunen
Copy link
Member

Havunen commented Oct 8, 2018

@simonjoom I Just published 6.0.0-rc.5 and babel-plugin 6.0.0-6 It fixes issue with empty fragments and when hydrating Fragments. Can you test if that resolved your issue. Thanks for all the help!

@simonjoom
Copy link

ok do that now :)
i have got a big project so i could find some bug for sure ))

@simonjoom
Copy link

simonjoom commented Oct 9, 2018

hello ok the issue of single fragment is still here unfortunately with the last version you asked me to try.

Nota: now it's returning [{object}, {object}] in the html not anymore null (i'm not sure it's can help...)

Anyway looking at the source code i found that in develop mode i ve got some:
<div class="app" __source="[object Object]" __self="[object Object]">

was the same thing than preactjs/preact#1058
I added pragma and this thing was resolved for me too ))

Because i use pragma I can confirm then that babel-plugin-inferno is well working for me.
i add below the main config i use with gatsby:


  if (process.env.NODE_ENV === `inferno`) {
   requiredPlugins.unshift(babel.createConfigItem([resolve(`babel-plugin-inferno`),{import:true,pragma: "h"}], {
    type: `plugin`
  }));
    }

  const requiredPresets = []; // Stage specific plugins to add
  if (stage === `develop`&&process.env.NODE_ENV !== `inferno`) {
    requiredPlugins.push(babel.createConfigItem([resolve(`react-hot-loader/babel`)], {
      type: `plugin`
    }));
  } // Fallback presets/plugins

  const fallbackPresets = [];
  const fallbackPlugins = [];
  let targets;
    
fallbackPlugins.push(babel.createConfigItem([resolve(`@babel/plugin-external-helpers`)], {
    type: `plugin`
  }));
fallbackPlugins.push(babel.createConfigItem([resolve(`@babel/plugin-transform-flow-strip-types`)], {
    type: `plugin`
  }));
 
  if (stage === `build-html`) {
    targets = {
      node: `current`
    };
  } else {
    targets = {
      browsers: pluginBabelConfig.browserslist
    };
  } 
  fallbackPresets.push(babel.createConfigItem([resolve(`@babel/preset-env`), {
    loose: true,
    modules: false,
    useBuiltIns: `usage`,
    targets
  }], {
    type: `preset`
  }));
  fallbackPresets.push(babel.createConfigItem([resolve(`@babel/preset-react`), {
    useBuiltIns: true,
    pragma: "h", 
    development: stage === `develop`
  }], {
    type: `preset`
  }));

In my webpackconfig i use alias:


const res=webpackconf.resolve;

res.alias["inferno"]="inferno/dist/index.dev.esm.js";
      res.alias["inferno-create-element"]="inferno-create-element/dist/index.dev.esm.js";
      res.alias["inferno-create-class"]="inferno-create-class/dist/index.dev.esm.js";
      res.alias["inferno-hydrate"]="inferno-hydrate/dist/index.dev.esm.js";
      res.alias["inferno-shared"]="inferno-shared/dist/index.dev.esm.js";
      res.alias["inferno-vnode-flags"]="inferno-vnode-flags/dist/index.dev.esm.js";
       res.alias['react-dom/server']='inferno-server/dist/index.dev.esm.js';
       res.alias['react']='inferno-compat/dist/index.dev.esm.js';
       res.alias['react-dom']='inferno-compat/dist/index.dev.esm.js'; 

@simonjoom
Copy link

simonjoom commented Oct 9, 2018

i found a bug from a code like that:
Nota for SideBarContext I use well create-react-context

 <SideBarContext.Consumer>
        {context => (
          <>
            {this.props.location.pathname !== "/" ? (
              <Button onClick={() => window.history.back()} icon="arrow_back" flat
              type="material"/>
            ) : (
              <Button onClick={() => context.toggleDrawer(true)} icon="menu" flat
              type="material"/>
            )}
          </>
        )}
      </SideBarContext.Consumer>

in looking in the source code:
Uncaught TypeError: Cannot read property '$LI' of null
at findDOMfromVNode (Users/simon/gatsby/material-starter- master/node_modules/inferno/dist/index.dev.esm.js:138)

function findDOMfromVNode(vNode) {
    var flags;
    var children;
    while (vNode) {
        flags = vNode.flags;
        if (flags & 2033 /* DOMRef */) {
            return vNode.dom;
        }
        children = vNode.children;
        if (flags & 8192 /* Fragment */) {
            vNode = vNode.childFlags === 2 /* HasVNodeChildren */ ? children : children[0];
        }
        else if (flags & 4 /* ComponentClass */) {
            vNode = children.$LI;
        }
        else {
            vNode = children;
        }
    }
    return null;
}

it's appear that the code didn't recognize the fragment and children is null for here
children.$LI;

If i change the fragment to normal div i ve got no issue at all

@simonjoom
Copy link

simonjoom commented Oct 10, 2018

ok again something to read:

I found a very strange behavior with inferno;
A input field builded with reactComponent who has got a type="password" don't event any onChange, if i change the field to a type="text" onChange then is working

I thought was my code but in normal mode (without inferno) i ve not this issue
Ok i found the problem
in inferno-compat:
change line if (!type || type === 'text') {

to if (!type || type === 'text' || type === 'password') {

@Havunen
Copy link
Member

Havunen commented Oct 10, 2018

@simonjoom

I found a very strange behavior with inferno;
A input field builded with reactComponent who has got a type="password" don't event any onChange, if i change the field to a type="text" onChange then is working

That was fixed by recent PR c7113bc

babel config:

[resolve(babel-plugin-inferno),{import:true,pragma: "h"}]

This will not work as you expect, because inferno JSX elements are compiled into different calls and not createElement, See https://github.com/infernojs/babel-plugin-inferno#pragma

 <SideBarContext.Consumer>
        {context => (
          <>
            {this.props.location.pathname !== "/" ? (
              <Button onClick={() => window.history.back()} icon="arrow_back" flat
              type="material"/>
            ) : (
              <Button onClick={() => context.toggleDrawer(true)} icon="menu" flat
              type="material"/>
            )}
          </>
        )}
      </SideBarContext.Consumer>

I have not seen that kind of function callback API, so I guess that's the issue here.

@simonjoom
Copy link

simonjoom commented Oct 10, 2018

ok nice that you fixed input type in c7113bc (even if it's still not released in your last version)

About pragma, i adjust it just to avoid some bad output in source code.
The Fragment problem is still here and even without pragma this code upper SideBarContext.Consumer is not working

even the simplest React component wrapped around fragment will still return anything in render.

Regards

@simonjoom
Copy link

simonjoom commented Oct 10, 2018

I found a bad behaviour using Fragment with inferno,
This work

<>
<App>
<Comp>
</>

But if i use fragment the behaviour is that all the components inside the fragments unmount (destroy) when i do a push history,
This behaviour conclude me to not use at all Fragment because i loose lot of thing (like csstransition)

so for now i recommand to turn all Fragment to div container

@Havunen
Copy link
Member

Havunen commented Oct 10, 2018

@simonjoom I cannot reproduce the issue you are having. ( Single component inside fragment )

Can you check this file what test case is missing?
https://github.com/infernojs/inferno/blob/master/packages/inferno/__tests__/fragments.spec.jsx

This example

<Sidebar.Consumer/> 

Will not work in InfernoJS because it needs new context API.

I might know what you mean with Router issue. So all components there re-mount right? I will look into that tomorrow, its most likely about normalization.

Thanks for the help!

@simonjoom
Copy link

simonjoom commented Oct 10, 2018

yes all remount
but this behaviour is certainly due the fact that i use a special component from gatsby that i bit modified for my case.

However with normal react i have not this unmount.

It's happened only after a change after state who modify the children components inside :

I don't put all the class here because it's very complicate but it's like a component who (re)render a component depending location change.

something like

class PageRenderer extends React.Component {
componentWillMount(){
this.wrappedPage= apigatsby(window.location)
}
componentDidMount(){
this.wrappedPage.then((end)=>{
    this.setState({wrappedPage:end})
}) 
}
componentDidUpdate(nextprops, prevState) {  
if(nextprops.location!==this.props.location){
     this.wrappedPage.then((end)=>{
    this.setState({wrappedPage:end})
})
}
}

 render() {  
    return this.state.wrappedPage 
  }
}

Maybe it's can help you to figure out or replicate the issue
the result of wrappedPage is a react.component who contain in render something like

render(){
return(<>
<App/>
<Test />
</>)
}

with Test:

class Test extends Component {
  componentWillUnmount() {
    console.log("unmountTest");
  }
  render() {
    return <div>Test</div>;
  }
}

I don't see any "unmountTest" on console when i run with react, with inferno i see "unmountTest"
All the tree seems to reload

And yes tested without fragment with inferno i don't see any "unmountTest"

About
"<Sidebar.Consumer/> " it's working for me i use many kind of consumer like that and it's working great.

I use @reach/router who use context api too it's working.
just the thing is to use the polyfill create-react-context

@Havunen
Copy link
Member

Havunen commented Oct 11, 2018

@simonjoom I have fixed the re-mount issue with Fragments, thanks for reporting it! Commit link here:
0de1223

even the simplest React component wrapped around fragment will still return anything in render.

For that use case I'm still missing steps to reproduce, do you know how to reproduce it?

@Havunen
Copy link
Member

Havunen commented Oct 11, 2018

Maybe you were using clone element and experienced this Bug #1393 ?

@Havunen
Copy link
Member

Havunen commented Oct 12, 2018

Okay, the issue occurs only when using createElement API, which means your babel plugin was not correctly configured, this is bug anyway and needs to be fixed. You can look into compiled output and if it uses createElement calls its not using inferno babel plugin.

@Havunen
Copy link
Member

Havunen commented Oct 13, 2018

I'm closing this ticket as Inferno v6 is now released! 🎉

@simonjoom if you still experience issues with Fragments you can create new Github ticket and we can continue there.

https://github.com/infernojs/inferno/releases/tag/v6.0.0

@Havunen Havunen closed this as completed Oct 13, 2018
@simonjoom
Copy link

simonjoom commented Oct 14, 2018

cool thanks.
To see my project working on live with gatsby & inferno v6.0.0
it's here: https://www.ski-courchevel.deals :) (in developement)

I'm closing this ticket as Inferno v6 is now released! 🎉

@simonjoom if you still experience issues with Fragments you can create new Github ticket and we can continue there.

https://github.com/infernojs/inferno/releases/tag/v6.0.0

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

No branches or pull requests