Skip to content

Fix wrapper.attributes() method #265

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

Merged
merged 1 commit into from
Dec 15, 2017
Merged

Fix wrapper.attributes() method #265

merged 1 commit into from
Dec 15, 2017

Conversation

tianyong90
Copy link
Contributor

Copy link
Member

@eddyerburgh eddyerburgh left a comment

Choose a reason for hiding this comment

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

Thanks so much for this PR, embarrassing that this slipped through. Could you please add tests. We need one for the wrapper, one for the wrapper-array, and one for error-wrapper

@tianyong90
Copy link
Contributor Author

Sure, I am starting working on it. 😄

@tianyong90
Copy link
Contributor Author

@eddyerburgh Hi, after reviewing the tests, I found that these needed unit tests are already exists and there's no need to change them,none of them failed.
image

@eddyerburgh
Copy link
Member

Hi @tianyong90 . These are bad tests if they passed when the code was buggy. Would you like to investigate why they were passing when the code was returning undefined? If not, I can merge and investigate myself. Thanks for the PR 😀

@tianyong90
Copy link
Contributor Author

image

Aha, Finally found out that it seems that it's a bug in the dist but not in src, perhaps there's something wrong with the bounding tools or configuration. Just like shown in the picture above, the spread operator is transformed to [].concat(), this is the reason why unit tests all passed, but broken in my actual use.
To fix it, you can just merge the PR, or make some change with the building process.

@eddyerburgh
Copy link
Member

Great find, thanks for the PR 🙂

@eddyerburgh eddyerburgh merged commit 1897300 into vuejs:dev Dec 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants