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

Send props not defined on the route component in $attrs. Fixes #1695. #1702

Merged
merged 2 commits into from
Oct 11, 2017

Conversation

lbogdan
Copy link
Contributor

@lbogdan lbogdan commented Aug 25, 2017

Fixes #1695.

@lbogdan lbogdan changed the title Fixes https://github.com/vuejs/vue-router/issues/1695 Fixes #1695 - not existing route component props are not passed as $attrs Aug 25, 2017
@lbogdan
Copy link
Contributor Author

lbogdan commented Aug 25, 2017

Hm, any idea why is the test failing? It works on my machine™:

 √ Testing if element <li a> has count: 5
...
 √ Testing if the URL equals "http://localhost:8080/route-props/attrs".
 √ Testing if element <.hello> contains text: "Hello attrs".

@LinusBorg
Copy link
Member

probably because nightwatch/phantomJS is a nightmre sometimes ...

someone with access to CircleCI can usually fix it by running it without caches.

@lbogdan
Copy link
Contributor Author

lbogdan commented Aug 25, 2017

Also, that test works locally, but this one (in test:types) doesn't:

types/test/index.ts(164,7): error TS2322: Type 'Component[]' is not assignable to type 'Component'.
  Type 'Component[]' is not assignable to type 'typeof Vue'.
    Property 'prototype' is missing in type 'Component[]'.

Which, if I look at the code, seems to be right:

const Components: ComponentOptions<Vue> | typeof Vue = router.getMatchedComponents();

(router.getMatchedComponents() should return Component[])

Strange?

@lbogdan lbogdan changed the title Fixes #1695 - not existing route component props are not passed as $attrs Send props not defined on the route component in $attrs. Fixes 1695. Aug 25, 2017
@posva
Copy link
Member

posva commented Aug 25, 2017

I rerun the tests

@lbogdan
Copy link
Contributor Author

lbogdan commented Aug 25, 2017

I see that my test passed, but as I said earlier, now test:types fails, even if my changes have nothing to do with TS code.

@lbogdan
Copy link
Contributor Author

lbogdan commented Aug 29, 2017

@LinusBorg @posva Any idea?

@vuejs vuejs deleted a comment from adi518 Aug 29, 2017
@posva
Copy link
Member

posva commented Aug 30, 2017

@lbogdan No idea unfortunately 😞
@ktsn Could you take a look, whenever you can, at the error in circle ci and see how it could be related to the small modification made 🙏 (Sorry for the inconvenience and thank you very much)

@ktsn
Copy link
Member

ktsn commented Aug 31, 2017

@posva
Looks like the type of Components variable in the test case is incorrect.

The router.getMatchedComponent() returns Component[] (alias of (ComponentOptions<Vue> | typeof Vue | AsyncComponent)[]) but it is annotated with ComponentOptions<Vue> | typeof Vue in the test case.

So replacing this line with the below would work.

const Components: (ComponentOptions<Vue> | typeof Vue)[] = router.getMatchedComponents();

@lbogdan
Copy link
Contributor Author

lbogdan commented Aug 31, 2017

@ktsn @posva Well, the test case is clearly incorrect, the question is why tests in other pull requests pass? 😄

@lbogdan lbogdan changed the title Send props not defined on the route component in $attrs. Fixes 1695. Send props not defined on the route component in $attrs. Fixes #1695. Aug 31, 2017
@ktsn
Copy link
Member

ktsn commented Aug 31, 2017

Looks like it's because typescript compiler version. TypeScript v2.4.0 passes the test cases but v2.4.2 doesn't. And the lockfile is updated recently. 76f6db9

@lbogdan
Copy link
Contributor Author

lbogdan commented Aug 31, 2017

@ktsn Makes sense, then. So, how should I proceed? Should I fix that in this PR?

@lbogdan
Copy link
Contributor Author

lbogdan commented Aug 31, 2017

Hm, still

"version": "2.4.2",
uses typescript 2.4.2, and the tests pass https://circleci.com/gh/vuejs/vue-router/1570 .

@ktsn
Copy link
Member

ktsn commented Aug 31, 2017

Maybe, it's because of CI cache?
I think we should fix the test case, anyway. I just made a PR for that. #1713

@lbogdan
Copy link
Contributor Author

lbogdan commented Aug 31, 2017

Ok, then I'll wait for it to be merged and rebase. Thanks!

@posva
Copy link
Member

posva commented Sep 1, 2017

@lbogdan I merged #1713

@lbogdan
Copy link
Contributor Author

lbogdan commented Sep 1, 2017

Finally! 😄

@lbogdan
Copy link
Contributor Author

lbogdan commented Sep 7, 2017

Ping?

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.

ping @yyx990803

@lbogdan
Copy link
Contributor Author

lbogdan commented Sep 20, 2017

Any news on this?

@lbogdan
Copy link
Contributor Author

lbogdan commented Oct 8, 2017

"Is there anybody out there?" 😄

@yyx990803 yyx990803 merged commit a722b6a into vuejs:dev Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants