A tour of how the Widget (Virtual DOM) code in Discourse works

Maybe start a new topic explaining exactly what you are looking to do, since I think changing the handlebars code is off topic for this virtual dom topic.

1 Like

Were you able to do this but with an array of multiple links? I’m trying and failing.

What code did you add? :slight_smile:

I was trying:

api.decorateWidget("hamburger-menu:generalLinks", _ => {
   return [ { route: 'birthdays', label: 'birthdays.title' }, {route: 'another', label: 'another.title' } ];
 });
1 Like

@eviltrout I may be missing something here, but it seems that it’s not possible for a widget to retain a state if it’s re-attached as part of a re-render.?

In other words, if you have a series of nested widgets (i.e. widgets attached to widgets), whenever one of the widgets triggers a re-render, the whole chain is re-rendered and any state in the child widgets is lost as they are newly re-attached. Is that correct?

For example when widgetizing Babble, I tried to set a default view state for the menu widget and then toggle it when the menu was toggled between a single chat stream and the list of chat streams, but found that the state would always remain at its defaultState. I surmised that the state was being reset when the menu widget was re-attached on a re-render.

I then tried to stop the ‘propagation’ of the re-render trigger, but wasn’t able to. I ended up using widget actions to send an updated state to the ultimate parent widget (i.e. the header) and retained the state in the header. See here.

If my assumptions are correct, it would be useful to have a way to stop the propagation of the widget re-render trigger, or a way to ‘turn off’ the re-render triggers for certain widgets.

For example this would be useful for the search-term widget, which is currently triggering the entire header to be re-rendered on every keyup in the search input. It would also be useful in various ways for Babble.

You are definitely missing something. The whole point of state is that it should remain across re-renders of the same widget!

One thing to check is that you have a buildKey method for every widget that has state. The key needs to be the same in order for the virtual dom to re-assign the state the next time it renders.

In the first releases of the virtual DOM code I didn’t enforce this, but I encountered enough bugs that I added a warning. Were you not seeing that warning in your development console?

2 Likes

Ah I see. I didn’t read this post carefully :blush:

Nope, I don’t think so. I’ll double check…

Can i add an ember component inside an widget. I want to a add login with google directly in place of log_in and sign_up buttons in header. So i was trying to directly call login-buttons component to be rendered over there.

You can do it, but there is a performance penalty from jumping back and forth between widgets and ember components.

See the connect method.

2 Likes

I understand that you are trying to use virtual-dom over normal templates by performance reason, but it makes writing and maintaining code much more complicated. Didn’t you think to use something more readable, for example JSX?

Yes of course I thought of it, but it would have been significantly more work to integrate a JSX -> virtual-dom compiler.

In the future we plan on using Glimmer 2’s handlebars compiler. It exposes an AST that we can use to build widgets, and then people will be able to use handlebars like for regular ember and for our topic lists.

7 Likes

If you do so, will you still support all “deprecated” compilers? I didn’t really understand yet how do you update Discourse, and how should I rely on using core functionality.

I’m not sure what you mean by deprecated compilers?

If you change to Glimmer 2, will you still support virtual-dom and old handlebars?

Yes, Glimmer 2 uses the handlebars syntax so the “old handlebars” will just work.

The virtual-dom code is currently not compiled, so Glimmer 2 will expose an AST that I will convert to virtual-dom, so old code will continue to work.

7 Likes

I’m using appEvents to trigger widget re-rendering after message-bus notifications. This is working ok, but I can’t find any way to “de-register” an appEvent listener once a widget is no longer required.

I setup the listener in the widget’s defaultState() function, in the same way as is done in core widgets:

https://github.com/davidtaylorhq/discourse-whos-online/blob/master/assets/javascripts/discourse/initializers/start-whos-online.js.es6#L85-L92

Ideally then I want to be able to call

appEvents.off("whosonline:changed")

when the widget is no longer needed. Otherwise the array of listeners keeps growing and eventually explodes

https://meta.discourse.org/t/whos-online-plugin/52345/90?u=david_taylor

I was hoping to use the widget destroy function, but it doesn’t look like it’s actually wired up to anything. The default implementation is a console message, and I don’t ever see that in the console.

https://github.com/discourse/discourse/blob/master/app/assets/javascripts/discourse/widgets/widget.js.es6#L176-L178

@eviltrout is there some way to run logic when a widget is thrown away? Or is there another way I should be approaching this problem?

4 Likes

Thanks for pointing out that the core widget is watching for an app event in defaultState - that is actually incorrect as it is never unloaded (cc @tgxworld) . In the case of search that might not be a problem since the widget is probably never unmounted but it’s not correct. (I’ve added it to my list to improve.)

The correct way to delegate events to widgets is by using dispatch(). For example:

https://github.com/discourse/discourse/blob/master/app/assets/javascripts/discourse/components/site-header.js.es6#L63

In that case, every time the notifications:changed event is fired, it will call notificationsChanged() in the user-notifications widget. After the method is called, it will automatically queue a rerender.

The event will be automatically removed when the widget is unmounted.

3 Likes

Update, it was definitely a problem, I noticed after using an autocomplete that the event was firing 20+ times! Fixed here:

https://github.com/discourse/discourse/commit/be1cce503c3576426ac9759d88fdcab4c0b87be2

5 Likes

Thanks - that’ll teach me to copy stuff from core :laughing:

So in terms of using dispatch from my plugin, I have modified the parent component that contains the post-avatar widget, to add the dispatch line.

   api.modifyClass('component:scrolling-post-stream', {
     didInsertElement() {
       this._super();
       this.dispatch('whosonline:changed', 'post-avatar');
     }
   });

I discovered that the second argument to dispatch is not the name of the widget, it is in fact the buildKey… This presents a problem for me. I need the event to be distributed to every instance of post-avatar, but obviously each user’s avatar is presented differently, so assigning the same buildKey to all of them makes very weird things happen.

Is implementing something like this the right thing to be do? Register/deregister for the appEvent in the parent component, set dirtyKeys to the buildKeys that require re-rendering, then queue a re-render of the whole post stream widget?

https://github.com/discourse/discourse/blob/1a02f5154fc1d479524e32928f421ebd19b46f6b/app/assets/javascripts/discourse/components/scrolling-post-stream.js.es6#L244-L256

Is there any way for me to set a wildcard dirtyKey, something like post-avatar-*?

2 Likes

Unfortunately you cannot refresh all widgets whose keys match a certain wildcard. You can only refresh all widgets * or do the trick that the scrolling post stream does there with the dirtyKeys if you know the ids of things you are changing.

Your solution of the appEvent in the parent component with the keys should work. I think a cleaner solution would involve re-engineering Discourse’s widget code a bit to add wildcards, or refreshing a widget by name instead of key.

4 Likes