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

feat(mixin): Automate event registration & removal on root vm #581

Merged
merged 29 commits into from Jun 30, 2017

Conversation

Projects
None yet
2 participants
@alexsasharegan
Contributor

alexsasharegan commented Jun 29, 2017

In response to Issue #569 and the conversation on PR #574

Alex Regan added some commits Apr 29, 2017

Alex Regan
Alex Regan
Merge branch 'master' of https://github.com/bootstrap-vue/bootstrap-vue
# Conflicts:
#	lib/components/form-fieldset.vue
Alex Regan
Alex Regan
Merge branch 'master' of https://github.com/bootstrap-vue/bootstrap-vue
# Conflicts:
#	lib/components/form-input.vue
* @link https://github.com/bootstrap-vue/bootstrap-vue/issues/569
*/
export default {
data() {

This comment has been minimized.

@tmorehouse

tmorehouse Jun 29, 2017

Member

I winder if this should this be a private prop where it is not "reactive"

This comment has been minimized.

@alexsasharegan

alexsasharegan Jun 29, 2017

Contributor

I don't believe private props are non-reactive anymore, but perhaps private notation is best.

This comment has been minimized.

@alexsasharegan

alexsasharegan Jun 29, 2017

Contributor

P.S. There's really an art to selecting the line number to comment on. This giant box can really get in the way!

This comment has been minimized.

@alexsasharegan

alexsasharegan Jun 29, 2017

Contributor

I'm not a fan of this method for non-reactive props because it has the potential to overwrite data.

This comment has been minimized.

@alexsasharegan

alexsasharegan Jun 29, 2017

Contributor

Also, where using Vue here, not Angular or React—if nothing is being rendered by that private prop, Vue won't do anything! Gotta love it

This comment has been minimized.

@tmorehouse

tmorehouse Jun 29, 2017

Member

True ;-)

This comment has been minimized.

@alexsasharegan

alexsasharegan Jun 29, 2017

Contributor

A little more info on _, $ prefixes and observation: vuejs/vue#665 (comment)

@tmorehouse tmorehouse added this to the v0.17.1 milestone Jun 29, 2017

@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Jun 29, 2017

I'm plugged this into my own project (seems like a useful tool), and it appears to be working nicely.

@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Jun 29, 2017

As you can see, the object is still observed event with private notation (this is slightly different naming). Maybe it is worth doing in the created hook? It's just a waste of observers and memory.
image

Alex Regan
Array.isArray = function(arg) {
return Object.prototype.toString.call(arg) === "[object Array]"
}
}

This comment has been minimized.

@alexsasharegan

alexsasharegan Jun 29, 2017

Contributor

Not sure if this is necessary, or if bublé will transpile Array.isArray

This comment has been minimized.

@tmorehouse

tmorehouse Jun 30, 2017

Member

You could just make a utility function isArray(...) that tries for the native Array.isArray first, and if not available then use the Object.prototype.toString(...) as a fallback, with out patching Array.prototype.

This comment has been minimized.

This comment has been minimized.

@alexsasharegan

alexsasharegan Jun 30, 2017

Contributor

Well, that answers that!

@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Jun 29, 2017

Now there's no observation done. I think this is in the best interest of performance. We can deal with some more abstract code in the lib if it's transparent to our users.

image

Alex Regan added some commits Jun 29, 2017

Alex Regan
Alex Regan
@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Jun 30, 2017

It's interesting; a lot of Vue blog posts talk about instantiating an empty Vue component and calling it EventBus, but that assumes a way for every component to access the EventBus. Apart from multiple Vue roots, this pattern of using root as the global bus makes more sense as it doesn't assume/require anything (perfect for a library).

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jun 30, 2017

Yeah, I have seen that done before, mainly for app communication, and global state management (instead of vuex)

@tmorehouse tmorehouse modified the milestones: v0.17.2, v0.17.1 Jun 30, 2017

@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Jun 30, 2017

I'm using vuex in my project, and while it largely eliminates the need for global events, there are still valid use-cases. For example, toggling a global "Are you sure?" modal (as you've seen screen captured). I also use this root event bussing to trigger the global login modal.

@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Jun 30, 2017

Just curious, any other refactor points? I'm about to dive into day job.

@alexsasharegan alexsasharegan referenced this pull request Jun 30, 2017

Closed

collapse::toggle::state triggered too many times #569

2 of 2 tasks complete
@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Jun 30, 2017

I think this is good now. Will merge shortly, then update my other pull request to take advantage of this.

@tmorehouse tmorehouse merged commit be5f834 into bootstrap-vue:master Jun 30, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@tmorehouse tmorehouse modified the milestones: v0.18.0, v0.17.2 Jun 30, 2017

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