-
-
Notifications
You must be signed in to change notification settings - Fork 130
refactor: Prefer event emitting to promises cancelation #370
Conversation
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.
Seems like a good alternative to the crufty solution in place now.
lib/basedriver/driver.js
Outdated
* The function may accept one argument, which is the actual error instance, which | ||
* caused the driver to shut down. | ||
*/ | ||
onUnexpectedShutdown (hanlder) { |
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.
handler
@imurchie I can also observe the TODO at https://github.com/appium/appium/blob/67e85ff02ce0d9d1237912e38b207869431e7e07/lib/appium.js#L411 which we could possible resolve by this PR |
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.
Excellent!
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.
yes, this feels a lot better. thanks.
According to appium/appium#13631 we still have memory leaks due to weird promise cancelation logic. IMHO promise cancelation itself is evil in its current implementation and I would rather avoid using it.
BREAKING CHANGE: The current refactoring is breaking and changes
onUnexpectedShutdown
to a normal event, which is emitted by the driver when it is shut down unexpectedly. I've also checked the depending drivers and it looks like only the umbrella driver uses this promise, so we'll have to change that implementation after the major version bump