-
Notifications
You must be signed in to change notification settings - Fork 257
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
POC: Plugin interface with wrapper in closure #82
Conversation
install(handler, options = {}) { | ||
if (typeof handler !== 'function') { | ||
console.error('plugin.install must receive a function') | ||
handler = () => ({}) |
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.
Should decide if we're going to exit the process here or quietly swallow the misconfigured plugin
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.
I think failing loudly is usually a good idea
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.
The louder the better, ideally with "you screwed up and here's where and how to fix it"
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.
Yeah. I'm fine with that. I wish I had a plugin name to point people at.
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.
hmmm didn't think of that. 🤔
it('receives the wrapper inside the plugin setup', () => { | ||
const plugin = (wrapper) => { | ||
return { | ||
$el: wrapper.element // simple aliases |
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.
Will we have access to private properties on the instance? Like the rootVM?
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.
Yeah, your original example will work. You can get access to the ComponentPublicInstance
under componentVM
which is private.
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.
const namePlugin = wrapper => ({ lowerCaseName: wrapper.componentVM.$.type.name })
config.plugins.VueWrapper.install(namePlugin)
const wrapper = mount({ template: `<h1>Hello</h1>`, name: 'My_Component' })
wrapper.lowerCaseName // 'my_component'
I resolved conflicts, and deployed latest master to my npm account and played around and so far this seems to work great 👍 nice job @JessicaSachs |
tests/features/plugins.spec.ts
Outdated
|
||
declare module '../../src/vue-wrapper' { | ||
// @ts-ignore | ||
interface VueWrapper { |
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.
@ts-ignore
should be removed here: you should just need to have the same signature than VueWrapper
, i.e VueWrapper<T extends ComponentPublicInstance>
and TS will be happy to merge the declarations.
You should also be able to remove all the other @ts-ignore
in the file.
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.
🤔 I will give this a try
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.
To no-one's surprised I changed the types based on @cexbrayat 's recommendation and it worked
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 to me! CI is passing and we have no conflicts. We need some docs then we can merge this (this seems like an important feature to document and document well).
I've been trying to keep docs up to date with vtu-next releases, just to make sure we don't document any POC or temporary feature (I think this is a sensible way of handling docs until we're able to merge repos). What I mean by this is that feel free to merge this up if you feel confident enough with the implementation and release a new alpha, and then we can work out the docs part :) |
Ok, let's do it. Great job all |
Nice! Thanks :-) I'll pick up the docs this weekend. |
This POC aims to add on to the current work surrounding plugins. I used the branch at #55 to build some plugins. I struggled with the existing API. I was limited from using fat-arrow functions due to the wrapper only being available on
this
. I wanted a closure where the wrapper would be passed in.Importance of Plugins
Our intension is that plugins will be a means for users to...
The pattern
Certain entities are Pluggable (VueWrapper, DOMWrapper). You can install plugins, which may return objects to be extended onto the instance of wrappers being mounted. This pattern looks like
data
orsetup
functions in Vue core and should feel familiar.Basic example
This plugin will add the
width
property andmyMethod
once the wrapper is mounted.Here is a plugin that will provide a new method to clean up the output of
text()
. (The use case is based off of jest-dom)Weak spots: