Use Vue root instance when injecting modals container #289
Conversation
| console.warn('[vue-js-modal] In order to render dynamic modals, a <modals-container> component must be present on the page') | ||
| let root = Plugin.rootInstance | ||
| if (params && params.root) { | ||
| root = params.root |
There was a problem hiding this comment.
I am not entirely familiar with the code, so might ask stupid questions.
is it intended that param.root overwrites Plugin.rootInstance?
There was a problem hiding this comment.
Don't worry, ask whatever you want :D. Yes, it is intended. As I explained in the documentation, in the unlikely case that someone has a Vue application with more than one root, it is possible that they want to open modals for different component trees. In which case, you would indicate which parent to use with this parameter (which would indicate what store to use or any other plugins).
src/index.js
Outdated
| if (options.injectModalsContainer && !root._dynamicContainer) { | ||
| const modalsContainer = document.createElement('div') | ||
| document.body.appendChild(modalsContainer) | ||
| new Vue({ |
There was a problem hiding this comment.
Should not new Vue(...) become a new rootInstance in this case?
There was a problem hiding this comment.
@euvl No, because the parent option is being used for this constructor, so it isn't considered root. You can read about that here: "The root Vue instance of the current component tree. If the current instance has no parents this value will be itself." https://vuejs.org/v2/api/#vm-root
| Vue.mixin({ | ||
| beforeMount () { | ||
| if (Plugin.rootInstance === null) { | ||
| Plugin.rootInstance = this.$root |
There was a problem hiding this comment.
This is pretty cool, I didn't know one can do it this way
There was a problem hiding this comment.
Yeah, quite frankly it's kind of "hacky" and I'm not entirely convinced of doing it this way. Because this will be called for every Vue instance, not only the "root". But there is really no way, as far as I know, to have a callback only when "root" instances are created, since a root instance is only a Vue instance without parents, there is nothing more special about them.
| }, | ||
| created () { | ||
| this.$modal._setDynamicContainer(this) | ||
| this.$root._dynamicContainer = this |
There was a problem hiding this comment.
This $root, is this the Vue instance? If so, I feel slightly worried that we write smth into it.
There was a problem hiding this comment.
Yes it is. I am using the _ prefix to have Vue ignore them, as explained here: https://vuejs.org/v2/api/#data The only problem I can see with this is if other plugin, or Vue itself, uses the same property name which I don't think it's likely. In that case, we could go even further and namespace the property, something like _vueJSModalDynamicContainer but I don't think it's necessary.
In any case, if you are not comfortable with this we can also store this in our plugin, having an object that maps from root instances to dynamic containers.
There was a problem hiding this comment.
Thanks for the explanation, i actually didnt know about _ and $ prefixed functions 😄
|
@euvl Ok I answered your questions. I don't have time now, but if you wait until this afternoon I'll commit the extraction of the lazy init into another function :D. |
|
@euvl Ok I extracted it to a function, let me know if it looks better now. Not sure if that's where the helper function should be declared, I thought about having it in util.js but it didn't look right and I wouldn't create a new file only for this. |
|
Cool, looks good to me 👍 |
2 participants
This fixes a point brought to attention by @joshualyon in PR #239. Essentially, modal containers were being injected without any parent, so accessing vuex's store or any other plugin in dynamic modals created like this was not possible.
The solution that I propose should be backwards compatible and work in most scenarios. The only exception is for applications that have more than one root Vue instance (which I haven't ever come across, but it is possible). In those cases, a new parameter
roothas been added and the root instance that is automatically registered (first mounted component) can be overridden usingrootInstanceattribute from the plugin.Since this has the potential to create more confusion than anything for newcomers, I'm not sure how to bring it up in the documentation. But it can also help mid-level Vue developers get more familiar with Vue internals, so I don't think it's entirely wrong to leave it there.
Let me know what you think.