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

[toggle directive] Root listener cleanup #680

Closed
alexsasharegan opened this Issue Jul 11, 2017 · 16 comments

Comments

Projects
None yet
3 participants
@alexsasharegan
Contributor

alexsasharegan commented Jul 11, 2017

We've had issues with using $root listeners before (#569), since the listener is not cleaned up by vue when the instance is destroyed. Chances are we could run into trouble with this directive listener: https://github.com/bootstrap-vue/bootstrap-vue/blob/master/lib/directives/toggle.js#L20-L24

@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Jul 11, 2017

I'm not sure how easy it is to unbind the listener, because it is generated with scope on the el

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 11, 2017

Might be able to do this on the unbind stage, as vnode is provided on unbind.

Would may also need to address the _taget bindings as well, although they are bound of the element.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 11, 2017

Hmm, it might be harder for toggle, as there is no this context to reference a handler specific to this element.

If we make a common handler function (outside of the export), then we loose the uniqueness of the handler that is associated with that particular element when trying to unbind the $root.on (it would end up removing all handlers associated the the event).

@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Jul 11, 2017

Exactly. The closure scoping is what really makes this messy. Can't save a reference because it has to be generated at binding time.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 11, 2017

The only way might be to create a "global" array of bindings in the directive file,, and somehow key it based on the node instance.

@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Jul 11, 2017

Yeah, I was thinking the same with a uniquely keyed dictionary, but either way, how to store the index/key of the handler? Maybe using a data attribute on the element?

@mosinve

This comment has been minimized.

Member

mosinve commented Jul 11, 2017

maybe it's a time to stop using $root as event bus? and implement standalone event bus. @tmorehouse, remember, that we discuss?

@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Jul 11, 2017

import { create } from "../utils/object"
import { uid } from "../utils"
const inBrowser = typeof window !== "undefined"

import target from "./_target"
const listen_types = { click: true }

// We only need a basic dictionary here.
let cache = create(null)

export default {
    bind(el, binding, vnode) {
        const targets = target(vnode, binding, listen_types, ({ targets, vnode }) => {
            targets.forEach(target => {
                vnode.context.$root.$emit("collapse::toggle", target)
            })
        })

        if (inBrowser && vnode.context && targets.length > 0) {
            const _uid = uid()
            el.setAttribute("aria-controls", targets.join(" "))
            el.setAttribute("aria-expanded", "false")
            el.setAttribute("data-v-bv-toggle-id", _uid)

            // Use named function to make debugging easier in stack traces.
            cache[_uid] = function toggleDirectiveHandler(id, state) {
                if (targets.indexOf(id) !== -1) {
                    el.setAttribute("aria-expanded", state ? "true" : "false")
                }
            }

            vnode.context.$root.$on("collapse::toggle::state", cache[_uid])
        }
    },

    unbind(el, binding, vnode) {
        vnode.context.$root.$off("collapse::toggle::state", cache[el.dataset.vBvToggleId])
        delete cache[el.dataset.vBvToggleId]
    }
}
@mosinve

This comment has been minimized.

Member

mosinve commented Jul 11, 2017

i wonder if the same can be done with weakMap or weakSet?

@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Jul 11, 2017

Totally! But that would add another polyfill.

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 11, 2017

Event bus would be nice as well, although would need to be injected into all components, and would change how users use emitting on $root to trigger events.

@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Jul 11, 2017

There is an inject API now. https://vuejs.org/v2/api/#provide-inject

I think it's a hard sell to require the extra setup for a user to implement a global bus. It wouldn't be hard, but it doesn't feel right to me (as a user in my app).

@mosinve

This comment has been minimized.

Member

mosinve commented Jul 11, 2017

Event bus may be assigned to Vue.prototype.eventbus at bootstrap-vue install function. and then it will be available at all Vue instances of the application. OR with provide/inject we can control the spread of eventbus just into needed instances

@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Jul 11, 2017

Would we need to namespace our own bus?

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 11, 2017

yeah, we probably should to avoid coflicts

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jul 15, 2017

What about placing a private property on the root vnode element? which would contain a reference to the listeners, or can we add a new piece of data in the component during bind? (i.e. __BV__root_listeners which would be an array of root listener events and code reference? maybe the same prop the listenOnRoot mixin uses?

tmorehouse added a commit that referenced this issue Jul 16, 2017

fix(toggle): Remove $root listener on unbind (Issue #680) (#698)
* fix(toggle): Remove $root listener on unbind

* Use named function for handler

@tmorehouse tmorehouse added this to the v0.19.0 milestone Jul 16, 2017

@tmorehouse tmorehouse closed this Jul 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment