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

Remove listeners after all requests are done #112

Merged
merged 1 commit into from
Jun 14, 2020

Conversation

mendelgusmao
Copy link
Contributor

I was scraping a page that does a lot of XHR requests and noticed this message

(node:81195) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 request listeners added to [Page]. Use emitter.setMaxListeners() to increase limit

This patch prevents the aforementioned memory leak by removing our listeners after all XHR are finished.

@jtassin
Copy link
Owner

jtassin commented Apr 26, 2020

Hi @mendelgusmao

This i s a great idea, thx.

I only have a problem with such feature with my application.
My application is a SPA with several waitForAllXhrFinished for a single new and this change will be a breaking change to the current behaviour.

Maybe we can and a parameter removeListener to waitForAllXhrFinished . And perform the removeListeners conditionaly.

@jtassin jtassin self-requested a review April 26, 2020 08:39
@mendelgusmao
Copy link
Contributor Author

@jtassin Your comment shown me that I was actually using your lib the wrong way by creating a new PendingXHR for every action expecting a XHR to happen. Thank you!

Anyway, here is a new proposal: waitOnceForAllXhrFinished(). It does the waitForAllXhrFinished as usual, then clean the listeners. I think this is a more OO approach.

@jtassin
Copy link
Owner

jtassin commented Apr 27, 2020

Anyway, here is a new proposal: waitOnceForAllXhrFinished(). It does the waitForAllXhrFinished as usual, then clean the listeners. I think this is a more OO approach.

OK for a new function waitOnceForAllXhrFinished that will remove listeners after itself 👍 I agree with you that the behaviour would be much better like this.

@sedenardi
Copy link

Hi @jtassin, any idea when you could push the new waitOnceForAllXhrFinished function to npm? I'm reusing puppeteer pages for scraping, and would like to use this to attach (and clean up) multiple instances of PendingXHR to the same page.

Thanks for your great work on this library.

@jtassin jtassin merged commit 7117d1e into jtassin:master Jun 14, 2020
@jtassin
Copy link
Owner

jtassin commented Jun 14, 2020

hi @sedenardi this feature is included in the 2.2.0 version

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.

3 participants