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

WIP: feat/fix: stub out transitions by default #1411

Merged
merged 2 commits into from
Apr 18, 2020
Merged

WIP: feat/fix: stub out transitions by default #1411

merged 2 commits into from
Apr 18, 2020

Conversation

lmiller1990
Copy link
Member

@lmiller1990 lmiller1990 commented Jan 20, 2020

This PR resolves #1384 and probably a bunch of other issues. Since <transition> causes problems, and there isn't really anything to unit test, I just stub them out permanently.

Edit: probably need the same thing for transition group. I'll add that.

I built and tested this against the reproduction provided and it passed.

@lmiller1990 lmiller1990 changed the title feat/fix: stub out transitions by default WIP: feat/fix: stub out transitions by default Jan 20, 2020
@@ -55,7 +55,7 @@ export default function createInstance(
const stubComponentsObject = createStubsFromStubsObject(
componentOptions.components,
// $FlowIgnore
options.stubs,
{ ...options.stubs, transition: true },
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt we add this to the default stubs in options? Wont it be a bit cleaner?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this might not be the best place to add this - which line/file do you think we should add this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Didnt we have a default global stubs object?

Copy link
Member Author

Choose a reason for hiding this comment

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

we do - updated the PR, good suggestion

@dobromir-hristov
Copy link
Contributor

Do you think ppl will want to have those transition hooks also working? I am not great with transitions, so I cant really say :/

@lmiller1990
Copy link
Member Author

lmiller1990 commented Jan 20, 2020

@dobromir-hristov I forgot about the JS transition hooks. I don't think it makes a lot of sense to unit test those. I think the amount of people having problems with the transitions is much greater than those unit testing transition hooks, so it's probably a good trade off.

If someone has a complex transition hook they need to test, we can probably work with them to find an alternative strategy to testing those.

@dobromir-hristov
Copy link
Contributor

Yeah you got a point, but lots of Vue UI libs use transitions, and ppl try to test for example if Modals are shown, or whatever, and some of those use the transition hooks.
I am just saying I am not exactly sure if this will be an easy thing for them to overcome.

@lmiller1990
Copy link
Member Author

Good point.

I'll run Vuetify's specs against this branch and see what happens. It looks like they already stub transitions.

Vue Bootstrap does use the those hooks, for example here. which will likely break their builds if they upgrade to the latest version. We could allow passing

stubs: {
  transition: false
}

for this case. Thoughts @dobromir-hristov ?

@lmiller1990
Copy link
Member Author

Update; I tried to upgrade both Vuetify and Vue Bootstrap's suites to the latest VTU but they failed. Without a lot of work upgrading those suites, there is no way to tell if this is a large, breaking change or not. For now, you can use the old behavior with

const wrapper = mount(Comp, {
  stubs: {
   transition: false
  }
})

I can update the docs to include this default stub if everyone else is happy with this solution.

@nachogarcia
Copy link

I'm stuck in v29 because we would have to rewrite our whole test suite to allow transitions etc. (Using bootstrap-vue). Which sucks because we have issues testing components that use code-splitting.

It would be really nice to have a built-in way of testing while using these libraries, although I understand how difficult is to integrate it. Just to express my point of view :P

@JessicaSachs
Copy link
Collaborator

@nachogarcia I believe this PR will help you with the transitions issue.

Agreed. We’re thinking about how to best serve people who use component frameworks.

@lmiller1990
Copy link
Member Author

I started upgrading Vuetify's specs so I could get them on the latest VTU, then bump to this and see if there are any major breaking changes due to this. I'm not sure it's worth it, though:

  1. upgrading Vuetify will take quite some time
  2. not reflective of all ecosystem (Vue Bootstrap etc)

Need a good way to handle this moving forward. Hm.

@lmiller1990 lmiller1990 mentioned this pull request Apr 3, 2020
13 tasks
@lmiller1990
Copy link
Member Author

Alright, it's time to head to v1. This bad boy is going in.

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.

v-if inside transition element not re rendering in beta30
4 participants