Skip to content

Finalize API #17

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

Closed
lmiller1990 opened this issue Mar 25, 2020 · 13 comments
Closed

Finalize API #17

lmiller1990 opened this issue Mar 25, 2020 · 13 comments

Comments

@lmiller1990
Copy link
Member

lmiller1990 commented Mar 25, 2020

Let's agree on a basic API and make an RFC so everyone can express their thoughts.

Proposed Wrapper API

method status notes
attributes keep
classes keep
exists keep
contains keep
destroy keep
emitted keep
find keep
findAll keep
html keep
name keep
trigger keep now returns nextTick. So you can do await wrapper.find('#button').trigger('click'). Nice!
setChecked keep see trigger
setSelected keep see trigger
setValue keep see trigger
vm keep
element keep

Wrapper API - Deprecations

method status explanation
emittedByOrder deprecate emitted serves this purpose
get deprecate This was added recently. similar to find - can we combine them?
is deprecate just use native tagName property
isEmpty deprecate Available via custom matcher, difficult to get 100% right
isVisible deprecate Available via custom matcher, difficult to get 100% right
isVueInstance deprecate
props deprecate Anti-pattern. Test what a prop does, not its presence or value.
setData deprecate Anti-pattern. Use data mounting option to set a specific state.
setMethods deprecate Anti-pattern. Vue does not support arbitrarily changing methods, nor should VTU
setProps deprecate See setData, setMethods
text deprecate use toContain

Mounting Options - Proposed API

method status explanation
data keep
slots keep
scopedSlots deprecate Slots are scoped by default in Vue 3. Use slots.
context deprecate No longer needed
stubs keep
provide keep
mixins new attach mixins by app.mixin, since you no longer attach these to Vue.prototype
plugins new attach mixins by app.mixin, since you no longer attach these to Vue.prototype
@dobromir-hristov
Copy link
Contributor

Great summary, here are my thoughts.

  • destroy -Keep- people use it to test destroy hooks and check for memory leaks, for example with continuous async functionality, like setTimeout loops for example.
  • emittedByOrder - Deprecate - I have never used it… We can just not implement for now, and add on a later phase if many ppl miss it
  • Is - Deprecate - I have rarely used it. Same as above.
  • isVueInstance - deprecate
  • name - Keep -it is used for both VM and DOM element name. I think its OK to keep it.
  • setData - Not Sure - I don’t use it. But I think Natalia gave a good reason to keep.
  • setMethods - Deprecate - I think ppl used it wrongly, as in to stop an async method from calling or something. Good docs and practices would mitigate this.
  • setProps - Not Sure - I have only used it to test watchers. In all other cases I just write a new test and mount. If we remove it, we need to show how to test such cases. Wrapping in another component just to update props might be too cumbersome.

Mount Options

  • attachToDocument - not sure.
  • context - Deprecate From the looks of it, functional components are just functions that accept the same params as the setup function. We should be able to just mount them as normal components, no?
  • scopedSlots -Deprecate- correct, they are now merged with slots.
  • mocks -Wait- there is a proposal, so we wait.
  • localVue -Deprecate- we can accept the app from createApp, its essentially the same purpose.
  • attrs - deprecate- sets properties on $attrs on the Component Instance. $attrs now holds everything, so it might be a bit trickier. We could just use props, to set any arbitrary prop, Vue will push it where necessary.
  • Listeners - not sure - We could prefix an event with on and set as a prop, to make their lives easier? It is essentially the same, right?
  • parentComponent - deprecate- It was to $parent, but Vue 3 removes that entirely I think?
  • provide -keep - Lets keep it provide, as its the same on the Vue Object Api? We can use the app from createApp to provide to the mounted component?
  • mixins - this is different from $prototype attachment. And there should be a difference between mixing globally into app.mixin and mixing in a component.
  • plugins - its for app.use correct? I think all app related methods could be prefixed somehow, or we let them provide their own app/vm instance.

@afontcu
Copy link
Member

afontcu commented Mar 25, 2020

Hey! :)
 👋

In face of doubt, I’d go with “deprecate”. I’d focus on having a single, simple way of doing things, and let userland provide additional modules/plug-in systems if needed.

So for instance:

setProps - Not Sure - [...] If we remove it, we need to show how to test such cases.

Totally in favor of deprecating + providing the right docs to showcase how to test such scenarions.


My two cents:

Assertion-like methods

We have several methods that are simply assertions. Could be remove them altogether – as long as users can assert the same stuff? Examples:

isVisible: deprecate. Visibility is really hard to get right. Jest-dom is closer than we are to achieve visibility checking: https://github.com/testing-library/jest-dom#tobevisible (which proves that the assertion can live in userland)

isEmpty: deprecate. Same thing as isVisible, even though is easier to get right (https://github.com/testing-library/jest-dom#tobeempty)

text: deprecate. What if we power up contains or even get so that they can accept strings or regular expressions?

Other methods / attributes

html: deprecate. What's the use case?

element: deprecate?. What's the use case?

and I’m gonna drop the bomb:

vm: deprecate. Can we provide a solid enough API so that people do not need to rely on the Vue instance? I’d like to consider it, at least.

@afontcu
Copy link
Member

afontcu commented Mar 25, 2020

setData - Not Sure - I don’t use it. But I think Natalia gave a good reason to keep.

If I recall correctly, docs didn’t cover that you can pass data directly, so that’s why they (gitlab) were using setData: https://discordapp.com/channels/527709196540575744/527714863238217732/670540697866076160

@lmiller1990
Copy link
Member Author

lmiller1990 commented Mar 26, 2020

I am keen to deprecate few things:

  • all set methods
  • isVisible and isEmpty

For vm and element

  • might be hard. I feel it is a bit of a disservice to completely hide these away from the user - maybe keep + undocumented, or as a "advanced users" with no guarantees.

I find html super useful, to see what is been rendered
Could live without text, it's basically just textContent, may make migration hard. We could keep with a deprecation warning.

I'll do a summary and draft an RFC.

@afontcu
Copy link
Member

afontcu commented Mar 26, 2020

all set methods

I think set methods were there because people had issues with v-model-powered inputs. Since v-model is being revamped, we might be able to remove them.

Could live without text, it's basically just textContent, may make migration hard. We could keep with a deprecation warning.

Same thing with classes() and Element.classList?


👍 with html, may come in handy when developing. Could we import it from the library itself?

import { mount, html } from '@vue/test-utils'

test('whatever', () => {
  const wrapper = mount(Comp)

  html(wrapper) // or similar API, whatever. it could also accept a WrapperArray

  // ...
})

@lmiller1990
Copy link
Member Author

I have not looked into the v-model changes much. @afontcu what kind of API do you envision for filling out forms, if not setChecked etc?

Importing html from the library could work - any reason you prefer this to having it on the instance?

I don't have a strong opinion either way, but we should be mindful some people have very large test suites they will want to migrate.

@afontcu
Copy link
Member

afontcu commented Mar 26, 2020

I have not looked into the v-model changes much. @afontcu what kind of API do you envision for filling out forms, if not setChecked etc?

If feasible, I'd go with a single setValue method that handles several form elements. Not sure if possible, though. We implemented a naive approach in VTL.

@lmiller1990
Copy link
Member Author

lmiller1990 commented Mar 26, 2020

That should be easy to implement - we just have setValue check the element type, then call setSelected, setChecked etc under the hood. Not sure is there is some complexity I'm missing.

I think this should be a (separate) RFC to the deprecation ones. A smaller, more simple API is definitely attractive to me.

@lmiller1990
Copy link
Member Author

I thought about setProps some more. One common use case is something like

<script>
export default {
  props: ['foo'],

  watch: {
    'foo': {
      doSomething()
    }
  }
}
</script>

People want to test that when foo changes, doSomething` is called. I personally never unit test this, I do it as a part of a larger integration test where this component is a child of some other component.

People like to test these in isolation, though. Is there some other way to test this other than setProps? I am very sure there will be huge pushback on this, we need to prepare something for when someone brings up this example.

@dobromir-hristov
Copy link
Contributor

Simulating setProps

Solution: Mount the component as a child of another component, and change the prop from there. Makes the test a bit more convoluted, but not impossible, explained it in my first response.

Clases, html, name etc

You want users to not have access to vm and other internals, yet you force them to dig around the returned instance to get the HTML, text or classes.
expect(wrapper.classes('thing')).toBe(true) vs expect(wrapper.vm.$element.classList).toContain('thing'))

Something to think about probably, or at least ask users for opinion.

setValue, setChecked and the like

I rarely use them personally, though I can see the appeal to if not remove, to reduce to a generic method.
v-model in Vue 3 has changed for custom components and could allow us to more easily trigger events on such inputs.

Example: I am testing my wrapper component around some Image SVG cropping tool. I should not need to dig inside the component to find which inputs to trigger an event against, as that could make my test fail if the developers changes internals. I should be able to just target the Vue component and setValue on it, right?

Maybe an API like this could work?

DomWrapper.setValue(value) => triggers a native DOM event depending on element type.
VueWrapper.setValue(value) => emit('update:value', value)
VueWrapper.setValue(value, 'text') => emit('update:text', value)

@afontcu
Copy link
Member

afontcu commented Mar 30, 2020

isEmpty -> keep -> Available via custom matcher, difficult to get 100% right

just keep be replaced with deprecate?

update: it should :) just updated the comment

@lmiller1990 lmiller1990 changed the title Finalize API + RFC Finalize API Apr 7, 2020
@afontcu
Copy link
Member

afontcu commented Apr 9, 2020

Should we close this up and wait for the public RFC to evolve? We can then match both our initial expectations and the outcome, and come up with a plan of features and releases 🙌

@lmiller1990
Copy link
Member Author

Sounds good

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

No branches or pull requests

3 participants