Inconsistent call of onrender

[nikku]

The generator allows target to be optional when instantiating a component (cf. https://github.com/sveltejs/svelte/blob/master/src/generate/index.js#L445).

It will always call onrender though, whether or not the component was mounted or not (cf. https://github.com/sveltejs/svelte/blob/master/src/generate/index.js#L461).

Expected Behavior

• On render is only called when the component is mounted to the DOM for the first time

Example

For a component like this:

FOO

<script>
export default {
  onrender() { ... }
}
</script>

the generator will currently create the following code snippet:

function SvelteComponent(options) {
       ...

	if (options.target) this._mount(options.target);

	if (options.root) {
		options.root.__renderHooks.push({ fn: template.onrender, context: this });
	} else {
		template.onrender.call(this);
	}

        ....
}

[Swatinem]

_mount is currently an implementation detail.
Public components should always have a target.

[Rich-Harris]

I wonder how we can enforce target being a required option – maybe if there's no target and no root we throw? Two options:

1. Change this line like so:

initStatements.push( deindent`
  var mainFragment = renderMainFragment( state, this );
-   if ( options.target ) this._mount( options.target );
+   if ( !options.root ) this._mount( options.target );
` );

That would cause an error when the compnent tried to append to a non-existent element, and it wouldn't add any code to components, but the error would be a bit mysterious.

2. We throw a useful error if both target and root are missing. Helpful, but adds code to every component

3. We do both, except that the useful error only appears in development mode (which doesn't yet exist – see Development warnings #13). Would require a bit of additional infrastructure and documentation.

I vote we start with 1 and then implement #13 soon, since there's lots of useful help we could give if we had a development mode.

[Rich-Harris]

Well, option 1 doesn't work, because components can be mounted immediately (depending on whether they are top-level or not). So, option 3 it is.

[nikku]

@Rich-Harris Don't quite get why 1) would not work. Can you elaborate on this?

[Rich-Harris]

If you have a top-level component...

<Widget/>

...it gets mounted after it's initially created:

function renderMainFragment ( root, component ) {
  var counter = new template.components.Widget({
    target: null,
    root: component.root || component,
    data: counter_initialData
  });

  // ...

  return {
    mount: function ( target, anchor ) {
      widget._mount( target, anchor );
    },

But if it's inside an element...

<div><Widget/></div>

...the component is appended to its containing element immediately, and the containing element gets appended later:

function renderMainFragment ( root, component ) {
  var div = document.createElement( 'div' );

  var counter = new template.components.Widget({
    target: div,
    root: component.root || component,
    data: counter_initialData
  });

  // ...

  return {
    mount: function ( target, anchor ) {
      target.insertBefore( div, anchor );
    },

So the presence of options.root (i.e. it's a component) doesn't mean it doesn't need to be mounted immediately, and vice versa.

[Swatinem]

Yes, and if its a child component, the onrender hook will be bubbled up to the parent, which makes sure the callback in invoked after the component is mounted.

Am I not really getting what you mean?

[nikku]

@Swatinem Just looking at the source code it reads as if target can be optional from the public API point of view. Reading through the comments that is not the case. Making that somehow obvious would help those consuming (and eventually debugging) compiled components.

[Rich-Harris]

As of 1.9.0, if you compile in dev mode (with dev: true), Svelte will check that top-level components have a target property, and throw otherwise. So I'll close this — thanks