Skip to content

Conversation

@leemiyinghao
Copy link

review?=@snowmantw
review?=@chungya
review?=@FlowerHop

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using an anonymous function, set the listener as this and handle events with handleEvent method is better.

@snowmantw
Copy link
Contributor

After check, your critical error look the same as 101502012's:

#45

Please correct it by yourself.

@snowmantw snowmantw mentioned this pull request Nov 7, 2015
@leemiyinghao
Copy link
Author

@snowmantw : I am not really understand what promise code should looked like, so I follow the code on page 93 of slide.
I'm not sure if he (or she?) have the same ref as me.

So I need to correct the error on Github? Or explain on the class next week?

@snowmantw
Copy link
Contributor

You only need to correct it on this PR. And the page is not about the error: it is only a general reference for what the flow should look like.

The point is you implemented the Promise version of XHR method at line 59, but you still keep the callback style that I expect you to refactor. So you can keep only the Promise pattern, and remove the callback.

Also please note the catch in Promise will capture the error, and then the rest part of the chain will never know that. So in order to know the error early, you should print the error in the catch section, and then you definitely should re-throw that in the same section after you just print it out. If you don't re-throw that, even there is a critical error in the chain before the catch function, the rest part will still execute and it could do some unexpectedly behavior. This is a common mistake among your classmates' code.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure if this is what Promise code should be like.
My understanding is pulling callback out from the function.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's unfortunately wrong. You just got trapped by a common mistake. In fact, you should do this if you don't want to use the plain chaining style:

promise = promise.then(....);
promise = promise.catch(....);

Please note it's because every time the then or catch is called, it will create a new instance of then-able. So if you do this:

promise.then(task1)
promise.then(task2)

It will NOT guarantee task2 is only after task1. In fact, they will execute concurrently. So you must make sure the then-able is updated as I wrote.

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.

2 participants