-
Notifications
You must be signed in to change notification settings - Fork 668
Add provide option to mount #3
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, although does it need two components? I think you can achieve the same effect with just component-did-inject.
<template>
<div>{{ foo }}</div>
</template>
<script>
export default = {
name: 'component-with-inject',
inject: ['foo'],
}
</script>
And then you can assert that foo is rendered
Can you make these changes?
also, vue-test-utils passes options to the constructor
Hey great point for discussion. Is provide/inject supposed to provide at the global Vue level or provide into the Component that we are wrapping? Right now in the mount we create Vue instance and a vm. That vm is what is getting the provided options, so unless i add the provide globally, i won't be able to both provide/inject from the same vm. |
After thinking about it, would be most valuable I think at global level bc then you could unit test injections without the cruft of an extra wrapper component. May need both? |
I think provide should provide globally for the instance. Like you said, you can unit test without a wrapper. I'll look into implementing it now |
Kk, I can also knock that out if you want to roll on to one of the other
todos.
…On Jun 4, 2017 9:32 AM, "Edd Yerburgh" ***@***.***> wrote:
I think provide should provide globally for the instance. Like you said,
you can unit test without a wrapper. I'll look into implementing it now
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABoXJPGZMiSDBm0L7M8p6SBnc_l95JDmks5sAr_ygaJpZM4NvQ0r>
.
|
Ok sure 👍 |
@eddyerburgh Alrighty so investigated a little further and have a few ideas. Wanted to get your thoughts on preferred direction before coding solution. So in component lifecycle, inject runs before the provide. What that means for us is that a component can't provide and inject into itself using standard vue stuff. https://github.com/vuejs/vue/blob/12b7122c161548bfc9865357d9b71302d66d4a9f/src/core/instance/init.js#L56 Couple options
Here is where the resolve for inject values happens. The lookup of the provide keys does start with the vm and then go to parents |
@Austio I think we should add Having a wrapper component feels too cumbersome to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice - looks great!
One last thing - provide can either be a function or an object. If it's a function you need to call it with this.
if (provide) {
vm._provided = typeof provide === 'function'
? provide.call(vm)
: provide
}
Can you add a test with a function passed to provide and update the addprovide to support it?
@eddyerburgh nice! Will get something around that as well for provide being a function |
@@ -0,0 +1,21 @@ | |||
function addProvide (component, options) { | |||
const provide = typeof options.provide === 'function' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/lib/addProvide.js
Outdated
@@ -14,6 +14,8 @@ function addProvide (component, options) { | |||
if (originalBeforeCreate) { | |||
originalBeforeCreate.call(this) | |||
} | |||
|
|||
component.beforeCreate = originalBeforeCreate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so....that was pretty hard to isolate as being the issue. My addProvide was mutating the beforeCreate in between tests. '
This is passing in chrome but not jsdom, will be able to check this out tomorrow :)
Brings me to a thought, when we mount things does it make sense to clone them or give the option for them to be cloned?
Nice find You're right, I think we should clone all components inside of mount if we're going to make changes to them. |
Actually, thinking about it - we shouldn't clone a component. That would cut all references to stubs. Is there a way to add provide without using the beforeCreate event? Can you add it to the root Vue instance so the component can take it from there? Then we can delete it after init has run. We're also planning on adding scoped Vue instances for each mount call, so editing the root shouldn't be a problem |
Good thought on avoiding clone. I dug though the _init source of vue early this morning and nothing jumped out as being an easy win. Will let my brain work in background and check on it again over lunch and then give a shot later. With the root vue instance idea, the issue is that the strategy for inject is the component checks itself and then loops through all Please let me know if you have any other thoughts/ideas. |
…it along with the other beforeCreate actions
@eddyerburgh just walked through every single step of initiation of Vue component and found that
|
It's because there's no circle.yml in your branch. I'm going to take your word and merge |
There was some linting errors, but the unit tests pass 🙌 I think since you forked I added linting to .vue files, so that's probably why the linting failed in master Thanks for the first PR of the project! 🎉 |
Wrote a failing spec to drive out the provide option and it passed :) Looks like vue-test-utils already supports provide/inject because it passes the options to the constructor in mount https://github.com/vuejs/vue-test-utils/blob/master/src/mount.js#L35
I think spec will still be good coverage for testing stuff in future.