Svelte: Inefficient teardown code?

[noypiscripter]

The compiled code seems to be not efficient in the teardown function. It keeps repeating the same if (detach) check for each node. If you have 100 nodes in a component, then you'll end up with 100 if (detach) checks instead of just one.

The compiled code below was taken from Hello World sample at https://svelte.technology/repl/?gist=0ed5146aa22c28410dfcff2050f3d2f8

teardown: function ( detach ) {
    if ( detach ) h1.parentNode.removeChild( h1 );    
    if ( detach ) text.parentNode.removeChild( text );      
    if ( detach ) text2.parentNode.removeChild( text2 );      
    if ( detach ) text3.parentNode.removeChild( text3 );      
    if ( detach ) p.parentNode.removeChild( p );      
    if ( detach ) text4.parentNode.removeChild( text4 );                  
    if ( detach ) text5.parentNode.removeChild( text5 );      
    if ( detach ) text6.parentNode.removeChild( text6 );      
    if ( detach ) text7.parentNode.removeChild( text7 );      
    if ( detach ) p1.parentNode.removeChild( p1 );      
    if ( detach ) text8.parentNode.removeChild( text8 );      
    if ( detach ) text9.parentNode.removeChild( text9 );      
    if ( detach ) pre.parentNode.removeChild( pre );                  
    if ( detach ) text10.parentNode.removeChild( text10 );
}

[Rich-Harris]

Yes, definitely – we should collect all those statements together. Shouldn't be that hard – would just need to have one block of non-detach teardown statements (removing event listeners, that sort of thing) and one block of detach statements. The generated code currently bears all the hallmarks of someone who doesn't yet fully understand the problem space just trying to get his damn code to work 😀

I'd kind of hoped that minifiers would be able to handle these cases smartly, but I'm not sure that's the case. So yeah, it's on us.

[mrkishi]

@Rich-Harris Would splitting generator.teardownStatements into two arrays be an acceptable solution, for now?

// teardownStatements: []
teardownStatements: { onDetach: [], always: [] }

I could work on that. 😊

[Rich-Harris]

basically yeah! though probably clearer if we do it this way instead:

+ detachStatements: [],
teardownStatements: []

I could work on that. 😊

🤘