[background] added events to ensure right page capture

[gastonche]

PR pertaining to #15 . I have added event listeners to all tab watching events that ensure if the tab url changes , it will reject the promise such that the watching action can't continue

[gastonche]

I will fix the corrections pointed out in the reviews, and update the PR, I added the tabChanged method which returns a promise that rejects if the tab is changed, that way, the function waiting for a resolve will stop. In that way we can avoid taking the snapshot of the wrong page @obsidianart

[obsidianart]

@gastonche The approach is clear, but I've never seen promises used for an event based pattern so I'd leave Treora decide

[Treora]

@obsidianart: It may not be very common, but promises are quite a fitting abstraction when waiting for an event to happen exactly once; especially when there is the need to stop waiting when another type of event happens. I wrote this small eventToPromise helper exactly because I ended up using this pattern several times.

@gastonche: looks like you got a technically sensible solution, roughly what I had in mind. I have a question the choice of events to listen to, and I think we can refactor the code to make it more understandable to a reader. Will comment inline.

[gastonche]

@Treora It would be nice to have it clearer for sure. What do you have in mind

[gastonche]

Okay @Treora , i'll try to refactor the Code in order to use the event to promise approach

[gastonche]

@Treora I have refactored the code to use the eventToPromise Approach. But then I left the new Promise in the whenPageDOMLoaded method because the browser.tabs.executeScript could not be added to the list of tabsChangedEvents since it dows not have an addlistener property and so will throw an error. I believe it's more readable this way.

[obsidianart]

@Treora I think event to Promise is a way to force a pattern within another, especially since you are waiting for rejection and not fullfilment. A promise rejection can come from many things (including syntax error). Comparing the following I find this approach quite singular, also considering Node has events emitter (https://nodejs.org/api/events.html). It might be my limit, but I always saw promises as a way to ask for something which you expect to happen, not to throw for something which you don't expect to happen.

//Promise
const waitForSmt = smt() 
waitForSmt.catch(doAction)

//Event
const waitFor = smt()
waitFor.on('smt', doAction)

//Callback
const waitForSmt = smt(doAction)

[Treora]

I think event to Promise is a way to force a pattern within another, especially since you are waiting for rejection and not fullfilment

@obsidianart: We are waiting for fulfilment. E.g. whenPageLoaded resolves when the page completed loading. It rejects if the page was closed before that ever happened.

I do understand and agree with your point that exceptions/rejections should be used for error handling, rather than for handling events that could be expected to happen. I am not sure if a page closing before it being loaded can reasonably be regarded as an exception.. For the user of the promise however, it probably does not matter if the promise rejected because of some really unforseeable error or because the page closed; either way what they waited for is not happening anymore.

I will think whether some event system can be more appropriate here, while still retaining the simplicity in usage, but I feel this promise approach is still acceptable.

[obsidianart]

Feel free to merge, I have no truth.

[Treora]

Great, looks ready to merge. @gastonche: would you be okay with waiving your copyrights on your contributions and publish it in the public domain?

[gastonche]

no problem @Treora . I am more interested in seeing the fixes in the repo