Add provide option to mount #3

Austio · 2017-06-04T03:19:44Z

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.

eddyerburgh

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

Austio · 2017-06-04T13:11:44Z

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.

Austio · 2017-06-04T13:47:11Z

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?

eddyerburgh · 2017-06-04T14:32:18Z

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

Austio · 2017-06-04T15:31:27Z

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> .

eddyerburgh · 2017-06-04T15:32:13Z

Ok sure 👍

Austio · 2017-06-04T19:54:38Z

@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

(safer imo) Mount the component inside of a parent component that provides the value and then uses the component as a child. There may be some things with this i'm not thinking about, we would want to return a wrapper around the child component still. Having a parent option was used a few times in vue-genome so we had a utility for it, was handy.
Set _provided before the initInject lifecyle hook

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
https://github.com/vuejs/vue/blob/b182ac40697edbe8253d4bd68b6ac09e93259e1c/src/core/instance/inject.js#L55

eddyerburgh · 2017-06-04T20:36:37Z

@Austio I think we should add _provided before init is called 👍

Having a wrapper component feels too cumbersome to me.

eddyerburgh

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?

Austio · 2017-06-05T13:08:08Z

@eddyerburgh nice! Will get something around that as well for provide being a function

Austio · 2017-06-06T02:38:17Z

src/lib/addProvide.js

@@ -0,0 +1,21 @@
+function addProvide (component, options) {
+  const provide = typeof options.provide === 'function'


Had to test here for if the provide is a function, otherwise the provide was turning into what appears to be an assertion.

…n in tests

Austio · 2017-06-06T04:52:09Z

src/lib/addProvide.js

@@ -14,6 +14,8 @@ function addProvide (component, options) {
    if (originalBeforeCreate) {
      originalBeforeCreate.call(this)
    }
+
+    component.beforeCreate = originalBeforeCreate


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?

eddyerburgh · 2017-06-06T06:19:10Z

Nice find

You're right, I think we should clone all components inside of mount if we're going to make changes to them.

eddyerburgh · 2017-06-06T06:43:07Z

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

Austio · 2017-06-06T14:28:24Z

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 $parent for a _provided. When we construct the component it is the root so $parent is undefined.

Please let me know if you have any other thoughts/ideas.

…it along with the other beforeCreate actions

Austio · 2017-06-07T02:22:17Z

@eddyerburgh just walked through every single step of initiation of Vue component and found that callHook will execute all of the events in a lifecycle and those events can be an array. Passing options.beforeCreate concats the function we create in addProvide to the list to be executed. So components beforeCreate runs and then ours does. Couldn't find any other way to get the data in there with current lifecycle unless we want to add an abstract parent component.

yarn run test passes locally in phantom and chrome for me. 🎉 Looks like failure in CI is due to npm install not running. If you want any help with the circleci let me know.

eddyerburgh · 2017-06-07T07:05:19Z

It's because there's no circle.yml in your branch. I'm going to take your word and merge

eddyerburgh · 2017-06-07T07:12:08Z

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! 🎉

Closes #3

Add test for accepting provide as an option to mount

72c47ca

eddyerburgh requested changes Jun 4, 2017

View reviewed changes

Austio changed the title ~~Add test for accepting provide as an option to mount~~ Add provide option to mount Jun 5, 2017

Austio added 3 commits June 4, 2017 22:28

Add provide option to mount.js

beefb16

Support original beforeCreate functions when providing value

24e842f

Move provide work to addProvide function

431cc9f

eddyerburgh requested changes Jun 5, 2017

View reviewed changes

Austio commented Jun 6, 2017

View reviewed changes

Austio added 2 commits June 5, 2017 22:39

Add provide function value and test

de333f0

Fix bug where component was re-using constructed beforeCreate functio…

caa8868

…n in tests

Austio commented Jun 6, 2017

View reviewed changes

Create options.beforeCreate in order to push it in handlers and call …

9b5e557

…it along with the other beforeCreate actions

eddyerburgh merged commit 51bc025 into vuejs:master Jun 7, 2017

eddyerburgh pushed a commit that referenced this pull request May 29, 2018

fix: Update links to docs (#670)

2f162e6

Closes #3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add provide option to mount #3

Add provide option to mount #3

Austio commented Jun 4, 2017

eddyerburgh left a comment •

edited

Loading

Austio commented Jun 4, 2017

Austio commented Jun 4, 2017

eddyerburgh commented Jun 4, 2017

Austio commented Jun 4, 2017 via email

eddyerburgh commented Jun 4, 2017

Austio commented Jun 4, 2017 •

edited

Loading

eddyerburgh commented Jun 4, 2017 •

edited

Loading

eddyerburgh left a comment

Austio commented Jun 5, 2017 •

edited

Loading

Austio Jun 6, 2017 •

edited

Loading

Austio Jun 6, 2017 •

edited

Loading

eddyerburgh commented Jun 6, 2017

eddyerburgh commented Jun 6, 2017

Austio commented Jun 6, 2017 •

edited

Loading

Austio commented Jun 7, 2017 •

edited

Loading

eddyerburgh commented Jun 7, 2017 •

edited

Loading

eddyerburgh commented Jun 7, 2017 •

edited

Loading

		@@ -0,0 +1,21 @@
		function addProvide (component, options) {
		const provide = typeof options.provide === 'function'

Add provide option to mount #3

Add provide option to mount #3

Conversation

Austio commented Jun 4, 2017

eddyerburgh left a comment • edited Loading

Choose a reason for hiding this comment

Austio commented Jun 4, 2017

Austio commented Jun 4, 2017

eddyerburgh commented Jun 4, 2017

Austio commented Jun 4, 2017 via email

eddyerburgh commented Jun 4, 2017

Austio commented Jun 4, 2017 • edited Loading

eddyerburgh commented Jun 4, 2017 • edited Loading

eddyerburgh left a comment

Choose a reason for hiding this comment

Austio commented Jun 5, 2017 • edited Loading

Austio Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Austio Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

eddyerburgh commented Jun 6, 2017

eddyerburgh commented Jun 6, 2017

Austio commented Jun 6, 2017 • edited Loading

Austio commented Jun 7, 2017 • edited Loading

eddyerburgh commented Jun 7, 2017 • edited Loading

eddyerburgh commented Jun 7, 2017 • edited Loading

eddyerburgh left a comment •

edited

Loading

Austio commented Jun 4, 2017 •

edited

Loading

eddyerburgh commented Jun 4, 2017 •

edited

Loading

Austio commented Jun 5, 2017 •

edited

Loading

Austio Jun 6, 2017 •

edited

Loading

Austio Jun 6, 2017 •

edited

Loading

Austio commented Jun 6, 2017 •

edited

Loading

Austio commented Jun 7, 2017 •

edited

Loading

eddyerburgh commented Jun 7, 2017 •

edited

Loading

eddyerburgh commented Jun 7, 2017 •

edited

Loading