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

fix(shared): toDisplayString toString override support #4337

Merged
merged 4 commits into from
Aug 16, 2021

Conversation

lidlanca
Copy link
Contributor

fix: #4334

fix for the case object without toString for example:

let obj = Object.create(null)  // typeof  is Object, however it will not have toString in prototype chain

handle the case that toString is non invokable, and use the JSON formatter for that case

let obj = {a:1, toString: 'bar')

improving on: #4335

@posva
Copy link
Member

posva commented Aug 15, 2021

Could you leave the reviews on the other PR instead of creating a new one, please?

@posva posva closed this Aug 15, 2021
@lidlanca
Copy link
Contributor Author

@edison1105 made a great pr, I worked on a fix before I saw your pr, and I hope that my PR did not offend you.
and since it was an additive pr, I kept it open for proper consideration.

@posva I would have appreciated you actually providing a review on this pr instead of closing it.
a lot of time and effort was put into this pr, and dismissing it is quite discouraging

@edison1105
Copy link
Member

@edison1105 made a great pr, I worked on a fix before I saw your pr, and I hope that my PR did not offend you.

and since it was an additive pr, I kept it open for proper consideration.

@posva I would have appreciated you actually providing a review on this pr instead of closing it.

a lot of time and effort was put into this pr, and dismissing it is quite discouraging

It’s ok. This PR covers more cases. Actually, I don't know if it's worth to check the override toString. Like @posva commented on other PR. The point is: should we put toDisplayString farther?let’s discuss it on other PR.

@posva
Copy link
Member

posva commented Aug 16, 2021

@posva I would have appreciated you actually providing a review on this pr instead of closing it.
a lot of time and effort was put into this pr, and dismissing it is quite discouraging

Once again, this is nothing personal, it's about respecting the original author's PR and making it easier to review for the maintainers as well (1 PR instead of 2 PRs) as well as discussing changes. You also seem to only see the things from your perspective, please also consider other people's perspectives 🙂

In this specific scenario, @edison1105's PR seems to be closer to what needs to be covered.

@lidlanca
Copy link
Contributor Author

haha "nothing personal", is like saying "I don't mean to offend but".
I recommend you let someone else review my contribution so you get more unbiased perspectives. :)

I actually do hope its personal, and that other people are not treated the same way.
who knows how many valid contributions were closed or dismissed because of demand for respect, lack of proper communication or other non professional reasons.

that is my perspective.

@yyx990803 yyx990803 reopened this Aug 16, 2021
@yyx990803 yyx990803 merged commit 3201224 into vuejs:master Aug 16, 2021
@yyx990803
Copy link
Member

I incorporated the isFunction check into #4335 and merged the test case from this PR. Thanks for the effort put into it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants