Decorators/hooks - Githubissues

Rich-Harris commented 7 years ago

A feature request that's come up once or twice — the ability to decorate elements with additional functionality, such as lazy loading images. Since 'using decorators' has come to mean 'smothering your code in @ symbols' it's probably no longer a great name. @TehShrike suggested 'hooks' which is pretty good, but this is open to bikeshedding.

Essentially, the idea is that you add a directive to an element and define a hook that gets invoked at runtime:

<div class='tall-element'>...</div>
<img hook:lazyload data-src='giant-photo.jpg'/>

<script>
  import 'intersection-observer'; // polyfill

  // this code is probably wrong, never used IntersectionObserver
  // but you get the gist
  var observer = new IntersectionObserver( entries => {
    entries.forEach( entry => {
      if ( entry.intersectionRatio ) {
        const img = entry.target;
        observer.unobserve( img );
        img.src = img.getAttribute( 'data-src' );
        img.onload = () => img.style.opacity = 1;
      }
    });
  });

  export default {
    hooks: {
      lazyload ( img ) {
        img.style.opacity = 0;
        img.style.transition = 'opacity 0.5s';
        observer.observe( img );

        return {
          destroy () {
            observer.unobserve( img );
          };
        };
      }
    }
  };
</script>

Or, passing data in:

<div hook:tooltip='hello!'>hover on me</div>

<script>
  import Tooltip from 'tooltip';

  export default {
    hooks: {
      tooltip ( node, data ) {
        function handleMouseover () {
          const tooltip = new Tooltip( node, data );

          function handleMouseout () {
            tooltip.remove();
            node.removeEventListener( 'mouseout', handleMouseout );
          }

          node.addEventListener( 'mouseout', handleMouseout );
        }

        node.addEventListener( 'mouseover', handleMouseover );

        return {
          destroy () {
            node.removeEventListener( 'mouseover', handleMouseover );
          };
        };
      }
    }
  };
</script>

Finally, updating data:

<span hook:sparkline='lastTenValues'></span>

<script>
  import Sparkline from 'render-sparkline';

  export default {
    hooks: {
      sparkline ( node, data ) {
        const sparkline = new Sparkline( node, data );

        return {
          update ( data ) {
            sparkline.update( data );
          },
          destroy () {
            sparkline.remove();
          };
        };
      }
    }
  };
</script>

Arguably the last one should be a component instead, but you get the idea.

One design challenge, which the last two examples gloss over: it doesn't feel like it makes sense to force the use of {{delimiters}} here (i.e. hook:sparkline='{{lastTenValues}}'), particularly because you might want to pass in an object and then you have a situation {{{ like: this }}} which just looks awful. But if we stipulate that in the context of this directive, the quotes are the delimiters, then if you want to use text then the second example doesn't really work — it would have to be this instead:

<div hook:tooltip='"hello!"'>hover on me</div>

Not sure if there's a clean way to resolve that.

I don't plan to work on this immediately, I think there are more pressing issues. Just wanted to have a place to discuss this.

PaulBGD commented 7 years ago

Well in the case of

<div hook:tooltip='"hello!"'>hover on me</div>

You do want to pass a string, so it makes sense that you have to declare it. As well if it supports passing multiple parameters, then doing something like

<div hook:tooltop='"hello!", 10'>hover on me</div>

would require you to declare strings.

evs-chris commented 7 years ago

As all bike sheds are meant to be green, I'd say hooks sounds a bit like there's a few predefined entry points that you can inject your own logic. Decorator is a pretty fitting name, but if you want to avoid confusion with proposed ES features, I like traits.

Construct name aside, I quite like <img as:lazyload data-src="giant-photo.jpg" /> and <div as:scrollspy="'myScrollSpyID'">...</div>. Side note: supporting template strings in unquoted expression-y attributes can help reduce quote pileup, since backticks don't quote attributes e.g. <div as:kajigger=string arg>...</div>. Some people with more exotic keyboard layouts have trouble with backticks, but ES has adopted it, so 🤷‍♂️.

It might also be handy to provide a few more lifecycle hooks to decorators. Perhaps a way to adjust the node before it's attached to the DOM. I suppose if the function was called before the node was attached, then a render or attach function would be more appropriate.

Rich-Harris commented 7 years ago

I do like as, yeah. It makes sense. Might pinch that from Ractive! Initially I thought it should align with the property on the exported object literal, and as: {...} looks weird, but then on: matches with events, and if we do transitions then it'll probably be in:fade, out:fade etc, so I'm not sure what I was thinking. 👍 to as. traits: {...} could work too, though possibly also burdened with existing connotations — was wondering about enhancers: {...}.

Backticks are a little tricky for strings with spaces, since syntax highlighters don't know what to make of them:

<div as:kajigger=`string arg`>...</div>

Doesn't look so bad with GitHub's colour scheme but is a bit nuts in my editor. I think @PaulBGD is right, we'd just have to quote strings (and maybe provide a useful error message for invalid expressions, saying 'did you mean to quote that?').

Quite like the simplicity of only having create, (optional) update and destroy. Since it all happens in the same tick, calling the function after the node is mounted to the DOM shouldn't result in any weirdness. But you raise a good point that is fundamental to the design, because if we get this wrong now it would be a difficult thing to undo — should the function be called before the node is mounted, or afterwards? What kind of setup work might need to happen before the node is mounted?

evs-chris commented 7 years ago

enhancers seems to fit nicely. :+1:

I think I may have been overthinking the need for a before and after attachment hook. Like you say, as long as whatever changes happen in the same tick as the attachment, you can prevent things like loading a giant image that's supposed to be lazy if you wanted the lazy enhancer to use src rather than data-src. I think the only exception would be script tags, which seem to evaluate immediately when attached, but that's probably far enough off in corner-case land not to matter.

TehShrike commented 7 years ago

Does it seem reasonable to allow decorators to be passed in to the component constructor, instead of always requiring to be imported in the component?

emirotin commented 7 years ago

Was about to suggest enhancers too

As for delimiters how about not having them at all? (Is Ember doing this?)

attr=42
attr='a string'
attr={ an: 'object' }

Is any major case missing? Variables interpolation?

TehShrike commented 7 years ago

I'm a lot softer on this feature now - I'm starting to believe that every single use case that you would use a hook for, you could/should use a component for.

<div class='tall-element'>...</div>
<LazyLoad>
    <img data-src='giant-photo.jpg'/>
</LazyLoad>

<script>
  import LazyLoad from 'lazy-load-component'
  export default {
     components: {
       LazyLoad
    }
  };
</script>

<!-- LazyLoad.html -->
<slot ref:img data-visible="{{visible}}" />

<script>
  import fireWhenReady from 'intersection-observer'
  export default {
    data: {
      visible: false
    }
    oncreate() {
      const img = this.refs.img
      const unsubscribe = fireWhenReady(refs.img, () => {
        img.src = img.getAttribute( 'data-src' )
        this.set({ visible: true })
      })

      this.on('destroy', unsubscribe)
    }
  }
</script>

This depends on #687.

TehShrike commented 7 years ago

The other possible route would depend on #640 (dynamic components), plus the ability to pass around element names as strings in place of components, plus #195 (spread).

<div class='tall-element'>...</div>
<LazyLoad component="img" data-src="giant-photo.jpg" class="my-cool-image" />

<script>
  import LazyLoad from 'lazy-load-component'
  export default {
     components: {
       LazyLoad
    }
  };
</script>

<!-- LazyLoad.html -->
<[component] ref:img data-visible="{{visible}}" {{...this.data}} />

<script>
  import fireWhenReady from 'intersection-observer'
  export default {
    data: {
      visible: false
    }
    oncreate() {
      const img = this.refs.img
      const unsubscribe = fireWhenReady(refs.img, () => {
        img.src = this.get('data-src')
        this.set({ visible: true })
      })

      this.on('destroy', unsubscribe)
    }
  }
</script>

emilos commented 6 years ago

The concept above seems to be really similar to behaviors from backbone.marionette that I've used in the past. The few use cases that I used them for and that they seemed ok for are:

tooltips (potentially component)
alerts (potentially component)
slide (potentially a method)

Benefits:

small reusable chunks of code (like mixins, decorators, shared code?)

Downsides:

it's harder to reason about the code (is this handled by a view or a behavior?),
yet another thing to learn,
weird use cases arise (e.g. how to hide the tooltip if the user scrolls and where to place that logic)

Imo components are flexible enough for all of the use cases above but maybe someone has better examples

hperrin commented 6 years ago

I'm with @TehShrike on this one. This can and should be done with other components, IMHO. The only thing I could see this causing a problem with is if you need a specific element to provide the functionality. As in, you have external code that relies on something being an img tag, but you have to wrap it with a LazyLoad tag. IIRC, Angular solved this by letting you define components using attributes. Would that be something Svelte could do?

jacwright commented 6 years ago

Drag and drop might be done better with hooks than components.

jacwright commented 6 years ago

I want this

I'm converting an existing app over to Svelte and finding a lot of places that my application uses the equivalent of hooks to add functionality to an element. These are some of them:

tooltips, can't implement with Svelte without hooks
autofocus, focuses the input when it appears on screen, native attribute doesn't work in SPAs
autoselect, (when the contents of in input get selected when it is focused)
SPA routing links
drop-down menu triggers (e.g. hook:menuTarget="refs.accountMenu")
drag, drop, and list reordering

Why just using components won't work

Too much abstraction makes things worse, not better

When you create a component to add functionality/behavior to an HTML element, you prevent other behaviors from being added to that element.

A great example is the <Link> component in svelte-routing. This component just needs to add a click listener to an anchor tag. But because it is a component, it needs to duplicate the attributes it guesses will be used (and has to use className since class is a reserved word). All use-cases can't be foreseen, so title was missed along with any custom attributes you might want such as data-tooltip which may be needed on the link if hooks/behaviors aren't supported. This means the consumer of the library must re-create their own Link component that supports the attributes they need, just so they can get access to that click listener logic.

On the other hand, if Link was a hook/behavior, it could be added with other behaviors to anchor tags without needing to proxy attributes across boundaries. Oh, and you could also add the behavior to buttons, div elements, whatever. You wouldn't have to rewrite a component because it didn't account for every use-case outside its concern of click.

I don't think components should be used for adding behaviors to existing structure. They should be used for new structures, combinations of multiple elements or single elements with set classes and functionality.

Technically not feasible

If it was decided components should be used for adding behavior, we would need control over slots for referencing what is in them. As this is a 0-many relationship, it does not fit the use-case of extending an element's functionality. E.g.

<slot ref:img data-visible="{{visible}}" />

In the above everything on <slot> is lost since slot is a space in the HTML, not an actual element. How could we translate this to zero or ten elements inside the slot?

Bikeshedding

While there is some precedence in other frameworks for using as, the word doesn't fit well. Since you are adding functionality to elements I like the word add better (and it only has 1 more character). See whether you think this reads better:

<button add:tooltip="t('create_project')">...</button>

<img add:lazyload data-src="giant-photo.jpg">

<div add:drag="dragOptions"></div>

<input add:autofocus add:autoselect>

<button add:menuOpener="refs:myMenu">Open</button><div class="menu" ref:myMenu>...</div>

For the actual name of the thing, I like behaviors more than enhancers. That might be too limiting but I can't think of using this for styles or something that I wouldn't call a behavior. traits is also nice. Hey, decorators works too. 😉

When to attach

They should be added after the elements are in the DOM. None of my examples actually modify the DOM when added, but the autofocus behavior/trait needs to call input.focus() after the element is in the DOM.

Curly braces, or not?

Not. All directives in Svelte do not use curly braces. It would confuse to change it up because the one use-case of add:tooltip="'Text'" looks nicer without. Consistency is more important. Besides, you can make your behavior support title as a fallback:

<div add:tooltip title="hello!">hover on me</div>

When can I have it?

So when are you going to finish this for me? 😄 Joking (not joking). I would love to have this today. I would be willing to take a stab at it if you think it would be a task within reach.

arxpoetica commented 6 years ago

Devil's advocate: I'm not convinced the functionalities you list can't already be done within the JS of the component. Example: autofocus can simply be done w/ a method or oncreate.

Advocate aside, if this does land, my vote is for smoosh

<div smoosh:tooltip title="hello!">smoosh on me</div>

</ducks>

jacwright commented 6 years ago

@arxpoetica is right about autofocus. You could use refs and just call focus on the component. All of these examples you could do the same using oncreate and ondestroy to set them up and tear them down. Unless they appear inside an #if or #each section, in which case you need to create components for each element that might have this extended functionality and you run into the problem I did with Link from the routing library where it doesn't support everything I need.

As I was asking for help/advice in implementing tooltips, @rob-balfre shared this with me https://github.com/subpx/svelte-jetpack/blob/master/src/modules/Shared/ToolTip/ToolTip.html for using TippyJS with Svelte. You must:

reference each element you are extending using refs or an id
add code in your oncreate and ondestroy for each element you are extending, which could become quite a lot if you have a lot of elements needing extension (anchors, form inputs, etc.)

This is where hooks/behaviors are a good idea. They clean up your component code a lot. Also, it helps a ton since you don't get create/destroy events for elements that are inside {{#if}} and {{#each}}. That could become very burdensome to try and add/remove functionality with elements as they are added/removed within a component.

I was able to make my tooltip and link-clicking solutions easier with event delegation. This is my App.html:

<script>
import { Route, initRouting, destroyRouting } from '../routing';
import { initTooltips, destroyTooltips } from 'dabble/toolkit/tooltips';
import Home from './home/Home.html';
import ProjectScreen from './project/ProjectScreen.html';

export default {
  oncreate() {
    initRouting(this.options.target);
    initTooltips(this.options.target);
  },

  ondestroy() {
    destroyRouting(this.options.target);
    destroyTooltips(this.options.target);
  },

  data() {
    return { Home, ProjectScreen };
  },

  components: { Route }
}
</script>

This is better since I don't have to "ref" every anchor tag or item with a tooltip on it. As you can see, I still need to tie into oncreate and ondestroy. But it isn't ideal.

Another problem I ran into was knowing when an element is removed. I had to add a MutationObserver on the current tooltip target so if it gets removed by Svelte while the tooltip is visible (e.g. if a click moves to another route) the tooltip isn't left hanging around on the screen. No mouseleave/mouseout events are dispatched on elements that are removed. If this was tied into Svelte's flow with hooks this would not be necessary since it would know when it was being removed from the DOM.

Summary: hooks/behaviors are composition, components are inheritance (except without the inheritance). We should totally provide behaviors.

smoosh is pretty good, but long. Could we make it smsh? :D

arxpoetica commented 6 years ago

I'm going to back off on this one since I think I'm in over my head, other than to say, this does feel like a substantial and a fundamental enough change to warrant thorough consideration. I would like to make sure we're not introducing something we'll later regret. (Not saying we will, I'm just citing the slow and steady target.)

The one rule it does seem to violate (which I think is what @TehShrike was trying to address--correct me if I'm wrong) is the notion that any given component should be in charge of its own thing, and not do something outside of itself. I.e., loosely coupled components in a sandbox, not tightly coupled to something outside of its own scope. So maybe enhancers/smooshers (🙃) are the right approach, but on the flip, like the <:Window/> tool, maybe such things ought to be loosely hooked by the component. You definitely bring up a few good use cases, such as drag & drop, that have me thinking.

Dunno. Just trying to provide an alternate voice. I'll be anxious to hear others' thoughts. I'm effectively going to watch in the wings now. Thanks. 😀

jacwright commented 6 years ago

Another thing, I think hooks will be beneficial to the Svelte ecosystem, making sharing code for Svelte apps that solve common problems easier.

jacwright commented 6 years ago

I gave it a stab and got it working great! I used add and behaviors. If those are not acceptable I can change it to as, smoosh, traits, decorators, whatever.

Conduitry commented 6 years ago

I think this can be closed now with the new 'actions' feature. Can reopen this if someone disagrees.

arxpoetica commented 6 years ago

I'm still confused about the need for this, so at the expense of continuing to be that obnoxious kid at the playground, I'm going to stick my neck out again. In this example:

https://github.com/sveltejs/svelte/blob/master/test/runtime/samples/action/main.html

Why not just do something like this?

https://svelte.technology/repl?version=1.58.3&gist=ed8049955fc5b9891bdc6beed134ed5f

I'm trying to be sincere about understanding the need. The former seems like a lot of unnecessary DOM manipulation at the cost of doing things the MagicSvelteWay™.

I promise I'm not trying to be obtuse.

TehShrike commented 6 years ago

For the tooltip example, if you had a whole bunch of tooltips on different elements, it would be annoying to have different event listeners and "should it be shown" variables for each one.

However, I do still believe that it would be preferable to make the component API cover that use case instead of adding new syntax. :-x I realize that this solution is a lot easier to get added to Svelte, though.

arxpoetica commented 6 years ago

Sure, and like I say, I'm not trying to be rude. I'm just pushing on the "is this really a good idea" front—like—perhaps too much, ha ha. My first posture (which I sort of admitted) is that I don't really understand it all, still, even after all this discussion. But the use case in the test link I gave above does a lot of DOM manipulation and that sort of just makes me think we're still headed down the wrong path.

For the sake of being a team player, I'm going to ceasefire now. I hope that y'all get the sense that I'm giving voice only to be an alternative voice for the sake of moving the needle in the right direction, even if that direction is in a way I don't get. 😹😹

Rich-Harris commented 6 years ago

Just to add one small example of where this would have helped me recently, I had a bunch of images that needed to be lazy-loaded. One way to solve that is a <LazyImg> component (which is what I ended up doing IIRC). But the problem was that some of my images needed border-radius: 50%, and some of them didn't, which meant I needed to add a new property on <LazyImg> just for that. You can imagine the ensuing combinatorial explosion if we needed to add borders or box shadows or filters or what-have-you.

An action eliminates that need. It's true that you could probably achieve the same thing with spread (speaking of which, I need to return to that branch! Lemme see if my brain will stay awake long enough for me to make some progress), but there are still drawbacks (for one thing, the action is less code and higher performance, theoretically).

Anyway, now that it exists, we have a way of comparing the two approaches in real apps — it may turn out that actions were a terrible mistake and we should remove them in a future version. My hunch is that they'll probably be a nice fit for a certain set of tasks though. And of course, if you don't use them you don't pay for them 😀

arxpoetica commented 6 years ago

Thanks everyone for being patient with me! Good work @jacwright. :)

jacwright commented 6 years ago

@arxpoetica thanks. And to illustrate another way actions are helpful, take your above example and put the button into a {{#each}}. It gets more complicated. You would probably then make the button a component, maybe called <TooltipButton>. This could work, and once spreads are implemented the downsides will be much fewer (right now this prevents you from adding attributes from the outside). But you will still be limited. You probably also want to create <TooltipLink> for links that need the tooltip. But what happens when you want to use svelte-routing which has its own <Link>? Again, once spreads are in place, you can add attributes, but you can't add other functionality. Probably should't wrap the two. You'll have to create a new component that brings in the functionality of both. TooltipButton, TooltipLink, Link, and TooltipRoutedLink. We're starting to get a lot of components to handle a bit of added functionality.

Actions aren't necessary, otherwise they would have been implemented from the start. But they do allow for easier code-reuse and better shared libraries without exploding/complicating the ecosystem. This is, I feel, the biggest problem actions solve, that of external libraries which add functionality to your app. It allows 3rd-party single-purpose components such as <Link> and <Tooltip> to become actions that can be combined within your app onto one <a> anchor. Or into your own <Link> component if you wish. And you don't have to be tied to using (or having to reimplement) someone else's <Link> component that doesn't allow you to do everything you need.

I've committed to using Svelte, so I want to see it become highly adopted and not die or lose steam. For that, I believe we need to build up the ecosystem. Shared components, actions, transitions, etc. which make it easier for ourselves and others to build apps. I feel this will help Svelte continue to grow and will make it a better tool for our jobs. I see actions as one of the needed pieces to make this happen.

ansarizafar commented 6 years ago

I have created an action for form validation and its really working well, Thanks to @jacwright and @Rich-Harris

<input use:validator bind:value='name' name="username" data-rules="required|max:30" class="input" type="text" placeholder="User name">

sveltejs / svelte

Decorators/hooks #469

I want this

Why just using components won't work

Too much abstraction makes things worse, not better

Technically not feasible

Bikeshedding

When to attach

Curly braces, or not?

When can I have it?