-
Notifications
You must be signed in to change notification settings - Fork 668
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
Conversation
@@ -55,7 +55,7 @@ export default function createInstance( | |||
const stubComponentsObject = createStubsFromStubsObject( | |||
componentOptions.components, | |||
// $FlowIgnore | |||
options.stubs, | |||
{ ...options.stubs, transition: true }, |
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.
Shouldnt we add this to the default stubs in options? Wont it be a bit cleaner?
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 this might not be the best place to add this - which line/file do you think we should add this?
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.
Didnt we have a default global stubs
object?
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.
we do - updated the PR, good suggestion
Do you think ppl will want to have those transition hooks also working? I am not great with transitions, so I cant really say :/ |
@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. |
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. |
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
for this case. Thoughts @dobromir-hristov ? |
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
I can update the docs to include this default stub if everyone else is happy with this solution. |
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 |
@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. |
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:
Need a good way to handle this moving forward. Hm. |
Alright, it's time to head to v1. This bad boy is going in. |
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.