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

Allow "listeners" to be passed into constructor #777

Closed
ScottMaclure opened this issue Aug 17, 2017 · 8 comments · Fixed by #794
Closed

Allow "listeners" to be passed into constructor #777

ScottMaclure opened this issue Aug 17, 2017 · 8 comments · Fixed by #794

Comments

@ScottMaclure
Copy link

Use case: I like to create my "app" component, then use app.on() API to bind event handlers on component events. Example:

var app = new App({
    target: document.querySelector('main'),
    data: someData
})

// Example listener outside the component hierarchy.
app.on('deleteUser', (event) => {
    // etc...
})

Problem: In a nested component's oncreate method I might call this.fire('foo') but it's too early to use app.on() with and I miss that event.

So... what if I could pass in a map of event names -> functions to the "app" component's constructor? Or is there a better approach?

@PaulBGD
Copy link
Member

PaulBGD commented Aug 17, 2017

This makes me think of a use case for adding a .mount method or something.

@Rich-Harris
Copy link
Member

Rich-Harris commented Aug 17, 2017

I can think of three possible solutions off the top of my head, each with downsides:

initialisation option

Per the initial proposal:

app = new App({
  target,
  on: {
    foo() {
      console.log('foo');
    }
  }
});

One weakness of this approach — though in practical terms, perhaps it doesn't matter — is that there's no equivalent of this:

app.on('foo', () => console.log('foo 1'));
app.on('foo', () => console.log('foo 2'));

Each event can only have one listener.

The other downside is that it necessitates adding generated code to every component:

if (options.on) {
  Object.keys(options.on).forEach(eventName => {
    this.on(eventName, options.on[eventName]);
  });
}

Not a lot of code, but we've been fairly careful about adding any non-optional code unless there's no other way. So let's see if there's another way...

Queued events

Some event systems allow handlers to receive events that have already happened. Not a huge fan of this approach, it's confusing.

.mount(target) method

This could work. Not much would need to change, and as a bonus you'd gain the ability to control where the component was inserted:

function App(options) {
  // ...
-  if ( options.target ) {
-    this._fragment.create();
-    this.mount(options.target, null );
-  }
+  if (options.target) this.mount(options.target, options.anchor);
}

// ...

+App.prototype.mount = function mount(target, anchor) {
+  this._fragment.create();
+  this._fragment.mount(target, anchor);
+};

The only thing I worry about here is API creep. Do we need to provide an equivalent unmount method? What's the behaviour if you repeatedly call mount? Does it add confusion about whether you're 'supposed' to use it (instead of specifying target), given that the only real use case is the one outlined here (not that you'd guess that from the API itself)?

Right now we have a very compact core API — three methods for dealing with data (set, get and observe, two for events (on, fire) and one for lifecycle (destroy). That's nice and learnable. Am hesitant to add to that unless we really have to. Which brings me to...

Secret option number 4

If initialisation options were passed through to oncreate unmolested, then you could add whatever you wanted:

app = new App({
  target,
  potato: () => {
    console.log('potato');
  }
});
<!-- App.html -->
<script>
  export default {
    oncreate(options) {
      options.potato();
    }
  };
</script>

This has the advantage of enabling things beyond this use case, including things we haven't thought of yet. And it's as simple as doing this in the generated code:

-var oncreate = template.oncreate.bind( this );
+var oncreate = template.oncreate.bind( this, options );

The disadvantage is that the component needs to be designed with that in mind — the oncreate handler needs to know that it might have to call the potato function. But for any situation in which an event was getting called inside oncreate, that's probably true anyway.

Would that solve your problem @ScottMaclure?

Related issue: #550

@Conduitry
Copy link
Member

Secret option 4 is something that I thought of and that I almost suggested as a reasonable way to do this, without having too much niche API - but from what I could tell, the oncreate methods of children components were called before that of the parent component, so any emitted events would still be emitted before the listeners were attached.

@Rich-Harris
Copy link
Member

from what I could tell, the oncreate methods of children components were called before that of the parent component

Wouldn't it only really matter for the top-level component though? You don't have an opportunity to attach custom properties to the options for child components.

Relatedly, I've wondered about special-casing on:create to solve exactly that problem:

<Widget on:create='doSomething()'/>

<script>
  export default {
    methods: {
      doSomething() {
        // just need to figure out whether this is called before the hook or after...
        console.log('this would be tricky to do any other way');
      }
    }
  }
</script>

@ScottMaclure
Copy link
Author

Regarding option 4 (and I think what Conduitry said), doesn't the order of execution mean app's oncreate is called after the app's event handler for the nested component's event? E.g: https://svelte.technology/repl?version=1.30.0&gist=615fc3182e53f9d49c312e70376b91f9

So, in the event handler method you can't call options.potato? Or am I missing something?

Also, can someone explain a bit more about how the "mount" approach would work, in regards to my use case?

If it helps, I could add more info here about what I'm trying to achieve in my app's design, to see if there's a better approach. Or I could post it on SO as a separate question?

@Rich-Harris
Copy link
Member

Or I could post it on SO as a separate question?

@ScottMaclure it sounds like we're ultimately talking about changes to Svelte itself, so this is an appropriate place to have the discussion.

I misunderstood the point about order of execution with nested components — thanks for the demo. What if options was attached to the component?

methods: {
  requestData: () => {
-    console.log('app requestData, options.potato() needed here?')
+    this.options.potato();
  }
}

Would that give us the necessary escape hatch?

@ScottMaclure
Copy link
Author

ScottMaclure commented Aug 19, 2017

Yes, I think it would!

Background to this use case: I've rolled my own routing, which is data-driven (e.g. app.set({ route: blah }).
My "app" component has if/else statements to control what "page" component gets rendered.
On a particular "page" component, oncreate fires a request up and out for its data, to be lazily loaded and set back in/down through the components.
I have some "store.js" equivalent modules that fetch data, outside of the components, loosely coupled thanks to usage of "app.on/set" API.

One use case breaks the design - when you refresh the browser with the URL pointing to this particular page - the app hasn't been created yet so the listeners aren't defined at that point.

I've written 2 demo apps in Svelte so far, following similar principles, and bumped into the same problem in both, so at least from my limited experience it sounds like a worthy issue to address.

The key technical design idea was to avoid injecting the store.js or other objects into the component hierarchy.... so this.options.potato() isn't terrible because I could give it some semantic name - I'm assuming that inside functions like that I'll be able to call app.set and my use case will work.

There are other approaches I could take for my app without needing a change to Svelte - for example, must my app actually wait until I navigate to that "page" component to lazily load data? What if I just start loading the data in the background, no matter what the page? Then I can just write a promise and call app.set, and the "page" component inits in a loading state. But would that scale if I had 50 similar pages in my app?

Rich, your HN demo in REPL has a similar feature with the async hashchange function, the only difference is I pulled logic like that out of the components completely.

Edit: here's an example nested component:
https://github.com/ScottMaclure/svelte-demo/blob/master/src/components/EditUser.html#L76

@Rich-Harris
Copy link
Member

As of 1.34, you can access this.options inside lifecycle hooks and methods

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

Successfully merging a pull request may close this issue.

4 participants