Skip to content
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

Discussion: Best way to create a HOC #6201

Closed
eu81273 opened this issue Jul 24, 2017 · 40 comments
Closed

Discussion: Best way to create a HOC #6201

eu81273 opened this issue Jul 24, 2017 · 40 comments
Labels

Comments

@eu81273
Copy link

@eu81273 eu81273 commented Jul 24, 2017

Version

2.4.2

Reproduction link

https://jsfiddle.net/o7yvL2jd/

Steps to reproduce

I've searching for the right way to implement HoC with vue.js. But couldn't found any suitable example.
The link below is known HoC implementations. But didn't work expected.
https://jsfiddle.net/o7yvL2jd/

How can I implement HoC with vue.js?
I just want to know how to implement HoC in the react.js way.

What is expected?

They are HoCs that simply renders components passed in as parameters.
HoCs containing slots and events will render normally.

What is actually happening?

The element to be rendered is missing, or the rendered order differs from the baseComponent.
Some HoC implementations do not work with event handlers.

@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Jul 25, 2017

Hello @eu81273

Thank your for your interest in this project.

However, your issue is a usage/support question, and the issue tracker is reserved exclusively for bug reports and feature requests (as outlined in our Contributing Guide).

We encourage you to ask it on the forum , Stack Overflow or on our discord chat and are happy to help you out there.

@LinusBorg LinusBorg closed this Jul 25, 2017
@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Jul 25, 2017

FWIW, I took a look out of personal interest - this should work 100% of cases.

const HOC = WrappedComponent => ({
  props: typeof WrappedComponent === 'function' // accept both a construtor and an options object
    ? WrappedComponent.options.props 
    : WrappedComponent.props,
  render (h) {
    // reduce all slots to a single array again.
    const slots = Object.keys(this.$slots).reduce((arr, key) => arr.concat(this.$slots[key]), []);
    return h(WrappedComponent, {
      attrs: this.$attrs,
      props: this.$props,
      on: this.$listeners,
    }, slots);
 }
});

I edited HOC04 in your example since it was the closest to the solution:

https://jsfiddle.net/Linusborg/o7yvL2jd/22/

Edit: still an issue with slots, investigating ...

@lbogdan
Copy link
Contributor

@lbogdan lbogdan commented Jul 26, 2017

I might have solved it: https://jsfiddle.net/BogdanL/ucpz8ph4/. Slots are just hardcoded now, but that's trivial to solve.

@blocka
Copy link

@blocka blocka commented Jul 26, 2017

Seems the solution is along the method of @lbogdan but createElement should have a way of taking slots, just like it can take scopedSlots.

However, it is still a lot of effort to create an HoC. There's a lot to remember to pass through, while with react you just render the WrappedComponent with props.

@blocka
Copy link

@blocka blocka commented Jul 26, 2017

I just thought of a very simple solution...let me know if I'm missing something here:

const HOC06 = WrappedComponent => Vue.extend({
   mounted () {
      console.log('mounted 6')
   },
   ...WrappedComponent
})
@eu81273
Copy link
Author

@eu81273 eu81273 commented Jul 26, 2017

Based on the examples given by @LinusBorg and @lbogdan, the most minimal HoC implementation that can handle components with slots is:

const HoC = WrappedComponent => ({
    props: typeof WrappedComponent === 'function' 
        ? WrappedComponent.options.props 
        : WrappedComponent.props,
    render (h) {
        const slots = this.$slots;
        const scopedSlots = {};
        Object.keys(slots).map(key => (scopedSlots[key] = () => slots[key]));

        return h(WrappedComponent, {
            attrs: this.$attrs,
            props: this.$props,
            on: this.$listeners,
            scopedSlots,
        });
     }
});

As @blocka mentioned, it is still a lot of effort to create an HoC with vue.js.

@blocka
Copy link

@blocka blocka commented Jul 26, 2017

@eu81273

const HOC06 = WrappedComponent => Vue.extend({
   mounted () {
      console.log('mounted 6')
   },
   ...WrappedComponent
})

This seems to pass your test, no? Of course, you would have to adjust it depending on whether WrappedComponent is a constructor or object, but no need to pass slots, events or props.

@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Jul 26, 2017

it is still a lot of effort to create an HoC with vue.js.

Apart from the issue with slots, this is just due to the fact that Vue does have a more complex API than React, which in this scenario is a disadvantage. I admire Reacts minimal API in these kinds of use cases - Vue was just designed with slightly different deign goals, so HOCs don't come as easily as in React.

But it should be fairly trivial to create a createHOC() helper function that wraps this initial setup for you, shouldn't it?

@lbogdan
Copy link
Contributor

@lbogdan lbogdan commented Jul 26, 2017

Well, it really depends what the end goal is. From what I understand, the goal of HoC is to somehow change (decorate) the original component (WrappedComponent) to add (or inject) props, methods, event listeners etc. (much like a mixin, really 😄 ). HOC06 variant only changes the component definition, it doesn't change the way in which it's instantiated.

@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Jul 26, 2017

@blocka The goal of HOCs often is to get state (e.g. from redux / vuex) and inject it into the wrapped component's props - that would not work with your approach.

@LinusBorg LinusBorg changed the title How to implement HoC correctly Discussion: Best way to create a HOC Jul 26, 2017
@LinusBorg LinusBorg reopened this Jul 26, 2017
@posva posva added the discussion label Jul 26, 2017
@blocka
Copy link

@blocka blocka commented Jul 26, 2017

@LinusBorg right. I knew it was too good to be true and that I was forgetting something obvious.

@lbogdan
Copy link
Contributor

@lbogdan lbogdan commented Jul 26, 2017

I think this a good example of implementing a real use case HoC in Vue: https://github.com/ktsn/vuex-connect.

@jackmellis
Copy link

@jackmellis jackmellis commented Jul 30, 2017

Vue Hocs would be an awesome plus (since it's almost always brought up in any vue vs react debate). Perhaps an official repo could be created to develop a vue-hoc-creator package? That way we could work on a robust, supported, implementation

@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Jul 30, 2017

Btw, got a better way: use $createElement from the parent component instead of the HOC's own - this make the child resolve the slots correctly:

https://jsfiddle.net/o7yvL2jd/23/

@blocka
Copy link

@blocka blocka commented Jul 30, 2017

@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Jul 30, 2017

I'm sorry for not coming up with an official solution yet. Glad you think it's cute though.

My solution is not perfect either, there are other problems to sort out - i.e. scoped slots won't work with my latest trick.

edit: oh, never mind, they work

An official solution will probably be done, at least I would expect so - but it has to be thorougly thought through and tested.

@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Jul 30, 2017

Okay, so I played around with a way to make this easier.

Have a look here: https://jsfiddle.net/Linusborg/j3wyz4d6/

I'm not happy with the API as it's a very rough idea sketch, but it's able to do anything it should be able to do.

import { createHOC, createRenderFn, normalizeSlots } from 'vue-hoc'
const Component = {
  props: ['message']
  template: `<p>{{message}}</p>`
}
const HOC = createHOC(Component)

createHOC will take care of:

  • copying the props from the component
  • swapping $createElement witht that from the parent for proper slots resolving.
  • add a name
  • if no second argument is provided (as in the above example), it renders the component and passes on:
    • props
    • attrs
    • listeners
    • normal slots
    • scoped slots

That on it's own isn't very useful, of course. So the fun happens in second argument which is a simple Component object.

If you want to write the render function on your own, you of course can. If you just want to extend props, attrs or listeners, you can use the createRenderFn helper. it will create a render function like the default one described above but will merge any attrs, props or listeners you pass to it with those from the parent.

const HOC = createHOC(Component, {
  mounted() { 
    console.log(`lifecycle hooks work, it's simply component options`) 
  },
  render: createRenderFn(Component, {
    props: {
      message: 'Hello from your HOC!'
    }
  }
})

If you want to write your own render function, you can use the normalizeSlots helper to transform the object from this.$slots into a proper array to pass on:

const HOC = createHOC(Component, {
  render(h) {
    return h(Component, {
      props: { message: 'hi from HOC'}, // nothing from parent will be passed on
    }, normalizeSlots(this.$slots))
  }
})

Comments wanted :)

@lbogdan
Copy link
Contributor

@lbogdan lbogdan commented Aug 1, 2017

@LinusBorg Very nice!

What I think would help is coming up with real life HoC use cases and solving them using these primitives.

@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Aug 2, 2017

Absolutely.

@AlexandreBonaventure
Copy link

@AlexandreBonaventure AlexandreBonaventure commented Aug 2, 2017

I'm mentioning this issue (EDIT (mybad): vuejs/vuejs.org#658).
Since you're using the undocumented $createElement API, It would be worth documenting it for plugin developers.

@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Aug 2, 2017

Your link is wrong (unless you really wanted to link to an issue from 2014)

But yes, technically the $ceateElement API is still missing on the API page.

@lbogdan
Copy link
Contributor

@lbogdan lbogdan commented Aug 2, 2017

@AlexandreBonaventure That issue is from vue 0.x days. 😄
Also, createElement is documented here: https://vuejs.org/v2/guide/render-function.html#createElement-Arguments .

@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Aug 2, 2017

The function is documented as an argument of the render function, but not that it is available via this.$createElement. There's an open issue for that on vuejs/vuejs.org/issues/658

@lbogdan
Copy link
Contributor

@lbogdan lbogdan commented Aug 2, 2017

@LinusBorg But it's basically the same function that gets sent to the render() function, right?

@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Aug 2, 2017

exactly the same. It's just not documented clearly that it's also available outside of the render function via this instance method.

@jackmellis
Copy link

@jackmellis jackmellis commented Aug 11, 2017

I'm just having a play with the above example and there are a couple of issues when using it in a more complex scenario, hence the need for an official repo. A couple of notes:

  • createRenderFn should check if attrs/props/listeners are functions and evaluate them, this would allow you to dynamically set props etc. based on existing props.
  • for composability should component be the second parameter, and if the entire createHOC method was curried we could easily chain multiple hoc creators together.
  • because the hoc is added as a mixin, if you try to chain 2 hocs together (i.e. withDefaultProps(withProps(component, {}), {}) the second hoc does not have access to the parent's props example
@jackmellis
Copy link

@jackmellis jackmellis commented Aug 19, 2017

Hey folks, I've been looking at writing an implementation of this.
https://github.com/jackmellis/vue-hoc/tree/develop
Let me know what you think and I'll look to publish it soon. I'm also thinking about writing a recompose-style package.

@blocka
Copy link

@blocka blocka commented Aug 19, 2017

@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Aug 19, 2017

@jackmellis Great that you take a lead on this :)

A couple of thoughts on your feedback you gave previously:

  • I think the curried version is a great point and should be the "default" in a way, as that'S how HOCs are generally done, aren't they?
  • Good point about the problem with mixins. Do you already have an idea how to mitigate this? I don't have one at this moment, but my gut feeling is that this should be mitigatable in a combination of the curried variants you created and using Vue.config.optionsMergeStrategies

I also thought about naming. I don't like createRenderFn, something like renderComponentWith would more meaningful and make more sense in a scenario where we embed it in some other nodes:

render(h) {
  return h('DIV', {staticClass: 'some-styling' }, renderComponentWith({ props: {... } }))
}
@jackmellis
Copy link

@jackmellis jackmellis commented Aug 21, 2017

  • In the end I've opted for both. So createHOC is component-first non-curried, and then there's a curried variant createHOCc. The React ecosystem is very functional-focused, unlike Vue, which acts more like an OOP framework. So I think it's best to keep consistent with the rest of Vue, but still offer a functional alternative.
  • I've just added some code to deal with this. Rather than having the whole hoc as a mixin, I manually merge the hoc and the options together using the optionsMergeStrategies. The only problem with that is that optionsMergeStrategies expects a vm as the last parameter, but I'm doing the merging before instantiation, so there is no vm.
  • I'm quite happy with createRenderFn since that's exactly what it's doing. But the more I put this together, the less I think people will use it directly. The general usage will be something like:
const hoc = createHOC(
  Component,
  {
    created(){
      /* ... */
    }
  },
  {
    props: (props) => ({
      ...props,
      someProp: 'foo'
    })
  }
);

Perhaps I don't quite understand your example?

@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Aug 21, 2017

Perhaps I don't quite understand your example?

No, I think you are on the right track, my thoughts were going in a similar direction when I thought some more about it after my last reply.

In my initial POC I included it so people could add additional markup around the rendered component, but that would mean it's not really a HOC anymore, as it would bring in UI as well...

@jackmellis
Copy link

@jackmellis jackmellis commented Aug 21, 2017

Yes I think you are attempting to render additional content, you've gone outside of HOC territory and might as well create an SFC to handle it.

@jackmellis
Copy link

@jackmellis jackmellis commented Aug 29, 2017

I've just published vue-hoc on npm!

I've also been working on vue-compose which is a quick documentation-session-away from being ready as well. Although it's similar to recompose, Vue handles a lot of clever stuff (like caching computations and it encourages the use of this) so it actually doesn't need quite as complex (or functional-style) syntax.

@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Oct 20, 2017

extends works differently, but can be used to achieve similar results, that's true.

I will close this since I think @jackmellis project does provide a solid foundation. We don't intend to include a way to create HOC's into core, mainly because we don't see a large benefit over mixins / Extends.

@LinusBorg LinusBorg closed this Oct 20, 2017
@njleonzhang
Copy link

@njleonzhang njleonzhang commented Dec 4, 2017

mainly because we don't see a large benefit over mixins / Extends.

@LinusBorg React have already abandoned mixin, HOC brings lots of benefit.

This article have analyzed the benefits:

  • The contract between a component and its mixins is implicit.
  • As you use more mixins in a single component, they begin to clash.
  • Mixins tend to add more state to your component whereas you should strive for less.
  • ....

I think Vue team should consider to support HoC more carefully....although it is not easy(It seems Vue is not designed in this way).

@LinusBorg
Copy link
Member

@LinusBorg LinusBorg commented Dec 4, 2017

I'm not convinced that HoCs are such a superior concept. The "implicit contract" potential clashes can happen with HoCs just as well, for example.

See this talk By the maintainer of React-router about it: https://www.youtube.com/watch?v=BcVAq3YFiuc

That being said I don't think they are bad either, they are useful in many situations. They are just not the magic bullet they are praised as in the React world, though, they have their own downsides.

As being evident from the discussion above, implementing HoCs in Vue isn't as trivial as in React because Vue's API and functionality is broader and more edge cases have to be taken care of.

We can surely talk about how to improve this as long as it doesn't require breaking anything in Vue - HoC's aren't worh a breaking change in my view.

@0x0006e
Copy link

@0x0006e 0x0006e commented Feb 28, 2018

666

@anson09
Copy link

@anson09 anson09 commented Sep 28, 2020

the most minimal HoC implementation that can handle components with slots is:

function hoc(WrappedComponent) {
  return {
    render(h) {
      return h(WrappedComponent, {
        on: this.$listeners,
        attrs:this.$attrs,
        scopedSlots: this.$scopedSlots,
      });
    },
  };
}

comparing to replys above,

  • props config is not need, which could be passed from this.$attrs to child
  • extra slots is not need, this.$scopedSlots contain slots since v2.6.0+

i writed a example to check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.