Skip to content
This repository was archived by the owner on Feb 13, 2024. It is now read-only.

Fix data parameter always undefined on callback for track method #231

Merged
pooyaj merged 5 commits into from
Feb 17, 2021
Merged

Fix data parameter always undefined on callback for track method #231

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d871c38
fix data parameter always undefined on callback for track method
Sep 23, 2019
b3d2c18
Add missing callback function to a remaining flush call
Jun 21, 2020
a64b2c9
Add callback to flush binding
albertininm Jan 11, 2021
85aba9f
Merge branch 'master' into fix-data-callback
nd4p90x Feb 1, 2021
b7f7b02
Merge branch 'master' into fix-data-callback
pooyaj Feb 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,16 +213,16 @@ class Analytics {

if (!this.flushed) {
this.flushed = true
this.flush()
this.flush(callback)
return
}

if (this.queue.length >= this.flushAt) {
this.flush()
this.flush(callback)
Copy link
Contributor

@Dahaden Dahaden Jun 30, 2021

Choose a reason for hiding this comment

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

Weird bug, may not cause issues, but this actually causes the callback to be called twice now as its the exact same callback pulled from the item when flushed.
https://github.com/segmentio/analytics-node/blob/master/index.js#L247-L259

Copy link
Contributor

@pooyaj pooyaj Jul 5, 2021

Choose a reason for hiding this comment

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

cc - @pbassut

Copy link

@BrandonNoad BrandonNoad Aug 12, 2021

Choose a reason for hiding this comment

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

Yes, the callback is running twice for me as well. Is there a plan to fix that? Or should I log a new issue?

Copy link
Contributor

@pbassut pbassut Aug 12, 2021

Choose a reason for hiding this comment

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

We should have a fix ASAP

Copy link

@BrandonNoad BrandonNoad Sep 29, 2021

Choose a reason for hiding this comment

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

Hi. Was the fix for this released with v5.1.0?

Copy link
Contributor

@pbassut pbassut Sep 30, 2021

Choose a reason for hiding this comment

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

@pooyaj It's fixed locally. I'm writing some tests and I'll create a PR for it.

Copy link
Contributor

@pbassut pbassut Oct 1, 2021

Choose a reason for hiding this comment

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

@BrandonNoad Can you give me a minimal working example that illustrates the issue?

For issue #1: seems unlikely given they're only called when the request to segment servers actually finishes(successfully or not).

As for issue #2: I'll have to see some code.

Copy link
Contributor

@pbassut pbassut Oct 1, 2021

Choose a reason for hiding this comment

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

@BrandonNoad Please open a new issue for it though.

Copy link

@BrandonNoad BrandonNoad Oct 1, 2021

Choose a reason for hiding this comment

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

@pbassut Yes, I can open a new issue with some code to reproduce the behaviour.

For issue #1: seems unlikely given they're only called when the request to segment servers actually finishes(successfully or not).

I may have been incorrect in saying "too early". That was just an assumption based on the fact that the callback is being called twice and in the first call, batch is undefined.

Copy link

@BrandonNoad BrandonNoad Oct 1, 2021

Choose a reason for hiding this comment

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

New issue is here: #300

}

if (this.flushInterval && !this.timer) {
Copy link
Contributor

@pbassut pbassut Jan 11, 2021

Choose a reason for hiding this comment

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

There's also another one here:

analytics-node/index.js

Line 217 in d2c565d

this.timer = setTimeout(this.flush.bind(this), this.flushInterval)

Copy link
Contributor Author

@albertininm albertininm Jan 11, 2021

Choose a reason for hiding this comment

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

thanks, @pbassut
just applied the suggestion!

this.timer = setTimeout(this.flush.bind(this), this.flushInterval)
this.timer = setTimeout(this.flush.bind(this, callback), this.flushInterval)
}
}

Expand Down