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

Component's public API #135

Closed
wants to merge 8 commits into from

Conversation

CyberAP
Copy link
Contributor

@CyberAP CyberAP commented Feb 26, 2020

Introduce a new expose option to declare component's public API.

Rendered

@CyberAP CyberAP changed the title Add component's public API RFC Component's public API Feb 26, 2020
@leopiccionia
Copy link

Wait, Vue 2 & 3 already has a provide option (the complement of inject...), with very different semantics.

Anyway, is it good idea to incentive the coupling between parent and child? There are good reasons for loose coupling being considered an OOP good practice.

About the RFC, I'd like to see a comparison with the use of interface (which obviously requires TS), more specifically, child components implementing interfaces and parent components depending on interfaces, not concrete implementations. Is it currently possible? Is it ergonomic to implement and use?

@CyberAP
Copy link
Contributor Author

CyberAP commented Feb 27, 2020

Wait, Vue 2 & 3 already has a provide option (the complement of inject...), with very different semantics.

Yes, current proposal is essentially extending provide behaviour.

Anyway, is it good idea to incentive the coupling between parent and child? There are good reasons for loose coupling being considered an OOP good practice.

The primary goal of this proposal is to reduce component coupling and make it as explicit as possible. When you have explicit public interface it can be refactored with ease and all your dependencies are tracked.

About the RFC, I'd like to see a comparison with the use of interface (which obviously requires TS), more specifically, child components implementing interfaces and parent components depending on interfaces, not concrete implementations. Is it currently possible? Is it ergonomic to implement and use?

With class API not officially supported I doubt it's feasible.

@leopiccionia
Copy link

Yes, current proposal is essentially extending provide behaviour.

Does it make sense that, to expose some data to my descendants (current behavior), I'd have to also expose it to my parent (proposed extension), and vice-versa? It seems leaky.

Another con of both sharing the same name is that provide can't/shouldn't be erased in production mode. I can't see why this sort of protocol/interface mechanism would need to exist beyond development.

I'd imagine, too, that providing data has some (probably rather small) runtime penalty, even if it's not injected by any descendant. I'd like to be proved wrong on this.

@CyberAP
Copy link
Contributor Author

CyberAP commented Feb 27, 2020

Does it make sense that, to expose some data to my descendants (current behavior), I'd have to also expose it to my parent (proposed extension), and vice-versa? It seems leaky.

That's listed in the drawbacks section of the proposal. I'd be happy with an alternative expose option if using provide for that purpose doesn't fit well.

Another con of both sharing the same name is that provide can't/shouldn't be erased in production mode. I can't see why this sort of protocol/interface mechanism would need to exist beyond development.

This interface should persist in the production mode. You should not be able to access component instance freely anymore.

I'd imagine, too, that providing data has some (probably rather small) runtime penalty, even if it's not injected by any descendant. I'd like to be proved wrong on this.

That's true if we have any injections, since they're recursive they will cycle through all the provisions of every component in the parent tree starting from the injecting component to the root.
I probably should state that in the drawbacks section.

@jacekkarczmarczyk
Copy link

jacekkarczmarczyk commented Feb 27, 2020

Not sure if the proposed RFC is the best way to do that but I do like the idea of explicit exposing public component's methods so I hope it's at least good starting point for discussion.
I think the expose thing mentioned in the last comment may be a better idea than using provide (and actually I had this name in mind before I read that comment)

@aztalbot
Copy link

aztalbot commented Feb 27, 2020

I like the goal - a lot actually. I was thinking about this very thing recently as a way to prevent foot guns like reaching into child element when unnecessary or testing internal state in unit tests.

I’m not a fan of the semantics, though. I would want to expose everything to the render context and pluck certain fields from that context to expose on the instance publicly, but I’m not sure overloading provide and using symbols is the best way. I view both of those things as maybe too advanced for some users who might want a really simple way to interact with a child component in a legitimate use case.

Maybe it doesn’t make sense, but I envisioned something like an expose option (like mentioned above) with a list of keys to expose on the instance publicly. In setup maybe there is a markExposed function and anything marked exposed on the render context object will be public, with all else only available on the context in render and template.

I envisioned something like:

const InputWrapper = {
    template: `<input ref="input" >`,
    expose: ['focus'], // parent can access the focus method
    methods: {
        focus() {
            this.$refs.input.focus()
        }
    }
}

or

function useFocus() {
    const focusable = ref(null)
    function focus() {
        focusable.value?.focus()
    }
    return { 
        focusable,
        focus
    }
}
const InputWrapper = {
    template: `<input ref="input" >`,
    setup() {
        const { focusable: input, focus } = useFocus()
        return {
            input,
            focus: markExposed(focus) // ensure parent can trigger focus
        }
    }
}
const InputWrapperParent = {
    template: `
        <InputWrapper ref="input" >
        <button @click="focus"></button>
    `,
    components: { InputWrapper },
    setup() {
        const { focusable: input, focus } = useFocus()
        return {
            input, 
            focus // triggers focus on the input wrapper, which focuses the element, but parent of this element cannot call focus on this component
        }
    }
}

These are just some ideas. I really like @CyberAP's idea; I'm just unsure about provide and symbols as the mechanism. Also, I am very curious about implementation and added type complexity (because I am guessing some mind boggling type definitions will be needed, although I hope not).

@CyberAP
Copy link
Contributor Author

CyberAP commented Feb 27, 2020

Thanks, @aztalbot, I'm leaning towards the expose more and more. I like the idea of simple API for quick exposure using an array of strings, the same way that inject works.

I was thinking that the expose API could have 3 versions each serving its own purpose:

  • Array of strings — quickly share data with direct bindings

    expose: ['foo', 'bar']
    
  • Object syntax — same as array, but with ability to rename exposed data

    expose: {
      foo: 'bar' // expose foo as bar
    }
    
    
  • Function syntax — full control over exposed data

    expose() {
      const { foo } = this
      return { foo }
    }
    

I was also thinking about mixins and $parent \ $root \ $children context access, should it be affected by this change or remain intact. I see the immediate benefits for mixin users, since we can control what data is exposed to mixins. With $parent I don't have a preference since I never use it.
What do you think?

@smolinari
Copy link
Contributor

smolinari commented Feb 27, 2020

Is the ability to dig into a child component's full context really an issue? If someone does it and does something improper or ends up even changing behavior, that's their problem in the end, isn't it? They are misusing the component.

Put another way, the API contract of a Vue component is implicit and doesn't need to be explicit. For instance, from the craziness I've seen, like some noob using private (as in with underscore prefixed) methods is very, very rare. And even when doing so, almost always things just don't work right or as expected. So, yeah, they may run into the footgun, but they back out of that attempt quickly and they'd usually learn to just compose a new component with the behavior they wanted.

I feel this is trying to fix a problem that doesn't exist, and the motivational points of the RFC don't really offer real world examples of the concerns either. Granted, this is coming only from my personal experience and perspective and even though I've been working to support and help people with a Vue framework for several years, I don't consider myself an expert.

As the saying goes, You can't design a system to avoid stupid. Stupid will always win.

Ok. I just made that up. 😁 But, I think you get the point and I feel overcomplicating the component API for a very rare problem is adding an unnecessary cost to the development of components.

Scott

@aztalbot
Copy link

one more thought, just because I anticipate some folks may want complete tree shaking, we could extract an api for a component using a helper:

import { createPublicComponent /* or some better function name */ } from "vue"

export default createPublicComponent({
    // nothing special here, no new options
    template: `<input ref="input" >`,
    methods: {
        focus() {
            this.$refs.input.focus()
        }
    }
}, ctx => ({ focus: ctx.focus })) // a callback to extract fields you want to expose to the parent, if no callback, exposes everything

@CyberAP, migration-wise, were you thinking a script could just find all usage $refs, and then add the necessary exports to that child component?

@smolinari I don't disagree with you. For this RFC to be viable in my mind, it needs to be easy to migrate and very lightweight. Otherwise the gain isn't big enough. Maybe the reason I see the benefit is because I have seen folks I know hit these foot guns recently so it's fresh in my mind. From a testing perspective, I do think having an explicit public API is helpful so it's clear what internals need to be tested and what does not (i.e. you don't need to tell folks not to test implementation details because those details literally wouldn't be accessible unless it's exposed for use by other components). But yeah, these are small component design wins.

@smolinari
Copy link
Contributor

Maybe coming up with some real world examples would be helpful? Explain where the implicit API wasn't enough and show how the suggested explicit API would be better. For sure that exercise will either strengthen your arguments and sell the suggestion better or maybe also change your mind on its overall value?

Scott

@jacekkarczmarczyk
Copy link

jacekkarczmarczyk commented Feb 27, 2020

Components can have a lot of methods. Some of them can be considered as public and others as internals. The main gain IMHO would be IDE autocompletion where you could see just the safe (public) ones. so take for example Vuetify VTextField component, it would be nice if after typing this.$refs.textField. you would get listed only focus and blur methods in autocompletion instead of endless list of internals. And hackers should be still able to access other methods, but using something like this.$refs.textField.$options.methods.genTextFieldSlot()

@CyberAP
Copy link
Contributor Author

CyberAP commented Feb 28, 2020

@CyberAP, migration-wise, were you thinking a script could just find all usage $refs, and then add the necessary exports to that child component?

@aztalbot, I'd better go with a warning in compatibility build, or some way of a loose mode, opposed to default strict mode.

I have rewritten the RFC to use exposed option and now it's open for reviews.

@CyberAP CyberAP marked this pull request as ready for review February 28, 2020 00:14
Copy link
Contributor

@smolinari smolinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really the best expert to evaluate the content. But, I did find a small mistake. 😁

Scott

active-rfcs/0000-component&#39;s-public-api.md Outdated Show resolved Hide resolved
@aztalbot
Copy link

I’m wondering if this is better left to userland implementation via plugins and composition functions.

I would think a plugin could add the expose option and also a refs option similar to https://github.com/posva/vue-reactive-refs/blob/master/README.md). The refs option could define validation and provide helpful errors in dev mode. When a component accesses a ref through this.$refs the only available fields would be the exposed ones (each ref would be proxied). For composition api, two helper functions expose(publicAPIObj) and childRef(refOptions) would provide this functionality. childRef or the refs option will throw an error in dev if the ref(s) it is populated with do(es) not match the validation options. For example, you might see a warning “Ref does not satisfy interface, missing focus method.“ I think this should provide static typing (with accompanying TypeScript) and dev runtime warning benefits without the huge changes likely needed to implement the rfc. Of course then you would need yet another plugin.

This might not totally address whatever I had mentioned about testing, but that can be addressed in a similar way I’m sure.

@privatenumber
Copy link

privatenumber commented Feb 28, 2020

Feedback

Great idea! I think tighter encapsulation of components is a step in the right direction.

Currently, a parent component can access and manipulate everything about a child component. As a part of a framework's role to guide engineers towards better practices, I think it makes sense to prevent things like letting a parent component directly manipulate a child component's state and vice-versa.

  1. Might be a good idea to update the first example to something that highlights a bigger problem. Currently, it expectedly calls a method on the child component, which doesn't highlight some of the more severe consequences of looser encapsulation (eg. anti-patterns to one-way data flow).
  2. If the purpose is tighter encapsulation, would properties like .$el have to be explicitly exposed too? I wonder what the implications this might have on the convenience of easily being able to access the DOM element to invoke API methods or retrieve data. And how far would we go to protect it when it can still be accessed via event objects?
  3. I think the only thing that should be exposed are methods or read-only data for the component to better enforce one-way data-flow and ownership of data.

Alternative considerations

publicMethods hash

Where only methods can be exposed and are defined in a separate hash

<template>
 <input>
</template>

<script>
export default {
 name: 'MyInput',
 publicMethods: {
   el() { return this.$el },
   focus() { return this.$el.focus() },
   blur() { return this.$el.blur() },
 }
}
</script>

Public method prefix

<template>
 <input>
</template>

<script>
export default {
 name: 'MyInput',
 methods: {
   // Public methods prefixed with $
   $focus() { return this.$el.focus() },
   $blur() { return this.$el.blur() },
 }
}
</script>

protected property

Similar to the functional property (I know Vue 3.0 removes it), but a component type would prevent any breaking changes (probably wouldn't be completely encapsulated and will still have to expose core properties like $el, $parent, $vnode, $options, etc.).

<template>
 <input>
</template>

<script>
export default {
 name: 'MyInput',
 protected: true, // Nothing is exposed unless explicitly defined below
 publicMethods: {
   el() { return this.$el },
   focus() { return this.$el.focus() },
   blur() { return this.$el.blur() },
 }
}
</script>

Custom block

Similar to above, would not introduce any breaking changes (unless people are using the api tag as a custom block)

<template>
 <input>
</template>

<script>
export default {
 name: 'MyInput',
}
</script>

<api> or <script api>
export default {
 el() { return this.$el },
 focus() { return this.$el.focus() },
 blur() { return this.$el.blur() },
}
</api>

@CyberAP
Copy link
Contributor Author

CyberAP commented Feb 29, 2020

@privatenumber thanks for your feedback! I have also considered the idea of sharing just methods to avoid directly manipulating other component's data. Probably the best way to deal with this would be printing an error in a console when you try to change component's state directly (stating that in order to change component's data prefer using exposed methods or passing down props), the same way it works with props mutations. Exposing just methods is too limiting in my opinion, since we can have computeds that could potentially contain important information about the component and to access those you'll need to call a method, which essentially duplicates existing functionality. But yes I believe everything exposed on a component should be read-only. Need more feedback on that issue before I add it to the RFC.

Regarding $el and other implicit props — I believe they should behave exactly the same as the rest of the context and require explicit exposure.

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the RFC.
I don't think this is solving any existing problem. The 3 points mentioned in motivation are very similar and are all solved by documenting what is accessible inside of a component. By default, specially when using ui libraries, no internal components properties should be used. This doesn't concern some properties like $el

@jacekkarczmarczyk
Copy link

Maybe it's enough there's an easy built-in way to properly type the component so that only public data/methods are available in the public interface?

@CyberAP
Copy link
Contributor Author

CyberAP commented Feb 29, 2020

Thanks for your input @posva.

By default, specially when using ui libraries, no internal components properties should be used

This is unfortunately not true. You'll have to access component internals in order to focus an element. Doing querySelector or $el are way worse than doing $refs.input.focus().

You'll also have to do this when using an uncontrolled component, like a self-closing dropdown. In order to forcefully close it you'll have to call $refs.dropdown.close() and there's no better way of doing it. You might use a watcher on a prop, but that's a reactivity hack, rather than a real solution to the problem.

And in case your component has these externally required methods there's no guarantee they will be present after the next refactor. If you do not specifically test for this then you'll even won't be able to tell that it's broken.

@posva
Copy link
Member

posva commented Feb 29, 2020

This is unfortunately not true. You'll have to access component internals in order to focus an element. Doing querySelector or $el are way worse than doing $refs.input.focus().

It is. That's not the component internals (data, methods, etc). It's part of a Vue instance. I literally mentioned $el right after in the sentence you cropped

You'll also have to do this when using an uncontrolled component, like a self-closing dropdown. In order to forcefully close it you'll have to call $refs.dropdown.close()

Yeah, and that is fine as long as it is documented. That's why I said by default, as in, if no documentation is provided.

And in case your component has these externally required methods there's no guarantee they will be present after the next refactor. If you do not specifically test for this then you'll even won't be able to tell that it's broken.

They will be there if they are part of the public api and there are documented. That's pretty much the whole point

@leopiccionia
Copy link

leopiccionia commented Feb 29, 2020

Maybe it's enough there's an easy built-in way to properly type the component so that only public data/methods are available in the public interface?

You can type this.$refs using TS interfaces. It's not the same of typing a child component directly.

The good parts are that interfaces are reusable and can be re-implemented by various components, and are totally erased during build (and, therefore, has zero runtime cost).

The bad part is that I don't know how to disallow $el access, for example.

See demo: https://github.com/leopiccionia/typing-vue-with-interfaces

Edit: I've tried to setup a CodeSandbox with Vue CLI and TypeScript support, but wasn't able to make it work.

@jacekkarczmarczyk
Copy link

See demo: https://github.com/leopiccionia/typing-vue-with-interfaces

Thanks, but I'd rather see a demo without using vue-class-component

@CyberAP
Copy link
Contributor Author

CyberAP commented Feb 29, 2020

The bad part is that I don't know how to disallow $el access, for example.

And even worse you can't disallow any access to the instance. There's nothing stopping you from doing vm.$refs.component.$emit('input', 'no-srp'). Or directly manipulating data.
I'm fine with closing down instance in development and completely disabling that in production. At least it will be very hard to do bad things while you're developing and consider finding alternatives or exposing data explicitly.

@leopiccionia
Copy link

And even worse you can't disallow any access to the instance.

At this point, it's important to settle which is the primary goal of this RFC:

  • Strict defensive programming (motivation 1);
  • Avoiding reliance on undocumented API (motivations 2 and 3).

Those motivations are quite different and affect distinct users sets, so the earlier we settle this, the more productive will be the RFC discussion from then on.

If we're only interested in avoiding unintended breaking changes and easing code refactoring, type checking + well-written documentation is a good enough combo. Linting rules can also go a long way to educate users about relying on other components' internals.

If strict defensive programming is critical, we should ask if something like expose is a productive solution, as $refs are not the only way to access component's instance externally AFAIK.

@CyberAP
Copy link
Contributor Author

CyberAP commented Mar 2, 2020

I wouldn't call that defensive programming but rather a contract programming.

@leopiccionia
Copy link

leopiccionia commented Sep 6, 2020

I created a POC for a way of defining a public API for use in Composition API, if anyone is interested in userland possibilities.

@yyx990803
Copy link
Member

Closing in favor of #343

@yyx990803 yyx990803 closed this Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants