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

ondestroy vs .teardown() and .on('teardown', ...) #365

Closed
Conduitry opened this issue Mar 11, 2017 · 5 comments
Closed

ondestroy vs .teardown() and .on('teardown', ...) #365

Conduitry opened this issue Mar 11, 2017 · 5 comments

Comments

@Conduitry
Copy link
Member

This might be weird functionality but it may just be a vague docs issue (or a me issue!).

So, recently the onteardown hook was renamed to ondestroy but there are still the .teardown() method on components and the .on('teardown', ...) event that that emits. Is the idea that the teardown event is only emitted when a component is manually torn down, or that it would be emitted any time a component is destroyed?

Are there any other events besides teardown that are automatically emitted? Should .teardown() and .on('teardown', ...) be renamed to match the onteardown -> ondestroy hook change?

@Conduitry Conduitry changed the title .ondestroy() vs .teardown() and .on('teardown', ...) ondestroy vs .teardown() and .on('teardown', ...) Mar 11, 2017
@Rich-Harris
Copy link
Member

Yeah, at the moment component.teardown() and component.destroy() are the same thing (and component.teardown() should probably be removed for version 2), but only a 'teardown' event is emitted. It should be renamed.

I propose the following:

For dev: false:

SvelteComponent.prototype.on = function on( eventName, handler ) {
+  if ( eventName === 'teardown' ) return this.on( 'destroy', handler );
  var handlers = this._handlers[ eventName ] || ( this._handlers[ eventName ] = [] );
  handlers.push( handler );
 
  return {
    cancel: function () {
      var index = handlers.indexOf( handler );
      if ( ~index ) handlers.splice( index, 1 );
    }
  };
 };

For dev: true:

SvelteComponent.prototype.on = function on( eventName, handler ) {
+  if ( eventName === 'teardown' ) {
+    console.warn( `The 'teardown' method is now 'destroy'. Update your application code before Svelte version 2` );
+    return this.on( 'destroy', handler );
+  }
  var handlers = this._handlers[ eventName ] || ( this._handlers[ eventName ] = [] );
  handlers.push( handler );
 
  return {
    cancel: function () {
      var index = handlers.indexOf( handler );
      if ( ~index ) handlers.splice( index, 1 );
    }
  };
 };

In both cases:

SvelteComponent.prototype.teardown = SvelteComponent.prototype.destroy = function destroy ( detach ) {
-  this.fire( 'teardown' );
+  this.fire( 'destroy' );

  this._fragment.teardown( detach !== false );
  this._fragment = null;

  this._state = {};
  this._torndown = true;
};

@Conduitry
Copy link
Member Author

Hmm, on lives in a helper, so there'd first have to be some sort of mechanism for those to vary between regular and dev modes. It's not entirely graceful, but I guess the first thing I'm thinking of is to have helper(name) append Dev to the name of the function that it brings in. There could either be a list of helpers that have dev versions that would have to be kept manually up to date - or the helpers could simply export both versions of everything even if they are identical, which shouldn't actually increase the compiled size.

@Conduitry
Copy link
Member Author

Pushed a WIP-but-should-pretty-much-work mechanism for having alternate dev versions of shared helper functions to the dev-helpers branch. There's a new devHelperLookup object (currently empty) that maps helper names to the names of their dev versions (which will also have to be exported from shared.js). Also tidied up a couple of other things related to helper function references. I'd be interested to hear whether you think this is the right track or not.

@Ryuno-Ki
Copy link

As a side node, I'd move the deprecation logic into a separate file, so you could maintain the formatting logic in one place.

I suggest an API like deprecate(this.name, "alternative", " X.y.z), which would console.log(\${old} is deprecated as of ${version}. Use ${new} instead!`)`.

@Rich-Harris
Copy link
Member

Fixed in 1.11.4.

Adding centralised deprecation logic is a little tricker in the context of Svelte, because sometimes it's a compile-time deprecation, sometimes runtime, etc. For now my position is YAGNI, but if we end up deprecating other stuff then it warrants revisiting

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

No branches or pull requests

3 participants