Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Fix flushInternal not succeeding #176

Closed
wants to merge 2 commits into from

Conversation

leyanlo
Copy link

@leyanlo leyanlo commented Jan 24, 2019

This was broken in #158. Calling this.flushInternal() within a setInterval() fails because this does is not bound properly like it is for this.flush(). The result is that logging is never flushed, and we see a continuous stream of error messages every 5 seconds:

2019-01-23T23:58:59.012Z - error: Cannot read property 'getAndClear' of undefined message=Cannot read property 'getAndClear' of undefined, line=null, col=null, stack=TypeError: Cannot read property 'getAndClear' of undefined
    at flushInternal (http://localhost:3000/shipper/_static/client-vendor.js:37174:32), message=Cannot read property 'getAndClear' of undefined, captureType=browser, captureSource=client, framework=fusion

Verified that running the plugin with these changes properly flushes, including flushing before termination.

KevinGrandon
KevinGrandon previously approved these changes Jan 24, 2019
Copy link
Contributor

@KevinGrandon KevinGrandon left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks!

@KevinGrandon
Copy link
Contributor

(We should see if we can write a test case to address this, maybe doesn't block landing though)

@leyanlo
Copy link
Author

leyanlo commented Jan 24, 2019

Added a test case for setInterval, and changed all UniversalEmitter methods to arrow functions for consistency

Copy link
Contributor

@KevinGrandon KevinGrandon left a comment

Choose a reason for hiding this comment

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

Hello, thanks for the pull request, and sorry about the delay in landing. It looks like someone from our team jumped on this and landed. (We should've landed from this pull request as a base).

Apologies about that. You have done some good work on the testing here, feel free to open up a new PR if you think the tests would be valuable to update.

@KevinGrandon
Copy link
Contributor

Relevant PR: #177

@leyanlo
Copy link
Author

leyanlo commented Jan 25, 2019

Ah no worries, thanks for the update!

@leyanlo leyanlo deleted the flush-internal branch January 28, 2019 21:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants