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

Promise returned by flush never rejects (and possible tests that do not work properly) #287

Closed
Dahaden opened this issue Aug 2, 2021 · 5 comments · Fixed by #294
Closed

Comments

@Dahaden
Copy link
Contributor

Dahaden commented Aug 2, 2021

Just noticed the latest changes includes the promise being returned by flush. Love it!

However, after looking at the code, I noticed that the catch swallows the error and then the promise is resolved with undefined.

This is easily testable by running something like

Promise.reject('test').catch(e =>
    console.log(e)
).then(() =>
    console.log('I end up in console.')
).catch(e =>
    console.log('I will not be logged.')
)

A fix to this is either:

  1. return Promise.reject(error) in the catch block, or
  2. throw error also inside the catch block

One big caveat to doing this though is that it may cause consumers to see Unhandled rejection errors if they do not catch these promises themselves.

I then had a look at the tests to see how to test this and saw https://github.com/segmentio/analytics-node/blob/master/test.js#L334-L346


test('flush - respond with an error', async t => {
  const client = createClient()
  const callback = spy()

  client.queue = [
    {
      message: 'error',
      callback
    }
  ]

  await t.throws(client.flush(), 'Bad Request')
})

which appears to test this exact functionality (even before the flush function returned a promise!)

I noticed that the dependency ava is quite out of date and bumped it to 3.15.0 and almost got everything working, but am now getting Unhanlded rejection errors.

Will push my branch up in a PR to show what I have done, but no idea how to fix this.

@nd4p90x nd4p90x added this to To do in analytics-node via automation Aug 13, 2021
@nd4p90x nd4p90x linked a pull request Aug 13, 2021 that will close this issue
@muhammadmahindar
Copy link

@Dahaden can you let me know if this will be fixed soon? I am facing the same problem.

Dahaden pushed a commit to Dahaden/analytics-node that referenced this issue Sep 3, 2021
… callback would have gotten. Removed pify from devDependencies
@Dahaden
Copy link
Contributor Author

Dahaden commented Sep 3, 2021

@muhammadmahindar Sorry almost forgot about this. Found what was causing me all the issues. New PR up with less changes in it
#294

@muhammadmahindar
Copy link

@Dahaden any timeline for when this PR will be released?

@Dahaden
Copy link
Contributor Author

Dahaden commented Sep 3, 2021

I dont work for Segment or have any permissions on this repo.

I have made a comment suggesting that this could be considered a major version bump due to the change in client.flush actually rejecting promises and the issue of causing some consumers to get many warnings or even throws errors.

@muhammadmahindar
Copy link

@Dahaden Thanks a lot dude. I think I'll have to clone until this is fixed.

@nd4p90x nd4p90x linked a pull request Sep 27, 2021 that will close this issue
nd4p90x added a commit to Dahaden/analytics-node that referenced this issue Oct 13, 2021
pooyaj added a commit to Dahaden/analytics-node that referenced this issue Nov 10, 2021
analytics-node automation moved this from To do to Done Nov 10, 2021
pooyaj added a commit that referenced this issue Nov 10, 2021
MoumitaM added a commit to rudderlabs/rudder-sdk-node that referenced this issue Jan 4, 2023
* Don't start a timer

When the queue has hit the maximum length don't start a timer. If a
timer exists flush will cancel it.

* Bump extend from 3.0.1 to 3.0.2

Bumps [extend](https://github.com/justmoon/node-extend) from 3.0.1 to 3.0.2.
- [Release notes](https://github.com/justmoon/node-extend/releases)
- [Changelog](https://github.com/justmoon/node-extend/blob/master/CHANGELOG.md)
- [Commits](justmoon/node-extend@v3.0.1...v3.0.2)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump eslint-utils from 1.3.1 to 1.4.2

Bumps [eslint-utils](https://github.com/mysticatea/eslint-utils) from 1.3.1 to 1.4.2.
- [Release notes](https://github.com/mysticatea/eslint-utils/releases)
- [Commits](mysticatea/eslint-utils@v1.3.1...v1.4.2)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump mixin-deep from 1.3.1 to 1.3.2

Bumps [mixin-deep](https://github.com/jonschlinkert/mixin-deep) from 1.3.1 to 1.3.2.
- [Release notes](https://github.com/jonschlinkert/mixin-deep/releases)
- [Commits](jonschlinkert/mixin-deep@1.3.1...1.3.2)

Signed-off-by: dependabot[bot] <support@github.com>

* fix data parameter always undefined on callback for track method

* Fixed a typo to reflect actual code behaviour

* Allow passing axios instance or config in options.

* v3.4.0-beta.2

* Update History.md

* Add missing callback function to a remaining flush call

* removed appended path from host

* allow to configure batch events path from initialization

* Bump elliptic from 6.4.1 to 6.5.3

Bumps [elliptic](https://github.com/indutny/elliptic) from 6.4.1 to 6.5.3.
- [Release notes](https://github.com/indutny/elliptic/releases)
- [Commits](indutny/elliptic@v6.4.1...v6.5.3)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump handlebars from 4.1.2 to 4.7.6

Bumps [handlebars](https://github.com/wycats/handlebars.js) from 4.1.2 to 4.7.6.
- [Release notes](https://github.com/wycats/handlebars.js/releases)
- [Changelog](https://github.com/handlebars-lang/handlebars.js/blob/master/release-notes.md)
- [Commits](handlebars-lang/handlebars.js@v4.1.2...v4.7.6)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump node-fetch from 2.6.0 to 2.6.1

Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1.
- [Release notes](https://github.com/bitinn/node-fetch/releases)
- [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md)
- [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1)

Signed-off-by: dependabot[bot] <support@github.com>

* Fix for infinite axios-retry on 5xx responses and axios client options leakage

* Fixing a formatting issue with test

* Enabled snyk for my account

* Added additonal unit test to prevent infinite axios retry from coming recurring

* Update history and bump the package

* Bump codecov from 3.5.0 to 3.7.1

Bumps [codecov](https://github.com/codecov/codecov-node) from 3.5.0 to 3.7.1.
- [Release notes](https://github.com/codecov/codecov-node/releases)
- [Commits](https://github.com/codecov/codecov-node/commits/v3.7.1)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump ini from 1.3.5 to 1.3.7

Bumps [ini](https://github.com/isaacs/ini) from 1.3.5 to 1.3.7.
- [Release notes](https://github.com/isaacs/ini/releases)
- [Commits](npm/ini@v1.3.5...v1.3.7)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump axios from 0.19.2 to 0.21.1

Bumps [axios](https://github.com/axios/axios) from 0.19.2 to 0.21.1.
- [Release notes](https://github.com/axios/axios/releases)
- [Changelog](https://github.com/axios/axios/blob/v0.21.1/CHANGELOG.md)
- [Commits](axios/axios@v0.19.2...v0.21.1)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump dot-prop from 4.2.0 to 4.2.1

Bumps [dot-prop](https://github.com/sindresorhus/dot-prop) from 4.2.0 to 4.2.1.
- [Release notes](https://github.com/sindresorhus/dot-prop/releases)
- [Commits](sindresorhus/dot-prop@v4.2.0...v4.2.1)

Signed-off-by: dependabot[bot] <support@github.com>

* update changelog and bump the version

* Bump acorn from 6.1.1 to 6.4.2

Bumps [acorn](https://github.com/acornjs/acorn) from 6.1.1 to 6.4.2.
- [Release notes](https://github.com/acornjs/acorn/releases)
- [Commits](acornjs/acorn@6.1.1...6.4.2)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump https-proxy-agent from 2.2.1 to 2.2.4

Bumps [https-proxy-agent](https://github.com/TooTallNate/node-https-proxy-agent) from 2.2.1 to 2.2.4.
- [Release notes](https://github.com/TooTallNate/node-https-proxy-agent/releases)
- [Commits](TooTallNate/proxy-agents@2.2.1...2.2.4)

Signed-off-by: dependabot[bot] <support@github.com>

* few dependency fixes

* remove size task

* remove more size

* pin yargs-parser

* Add callback to flush binding

* throws error when message is over 32kb

* Fixing conflicts

* releasing a major version

* using axios instance

* calling post method as its more testable

* testing if we can use the custom axios instance

* Release a patch version to fix the issue with axios instance

* upgrade uuid dep to support bundler tree shaking

See https://github.com/uuidjs/uuid/blob/master/README_js.md#deep-requires-now-deprecated

From the above document, the motivation for upgrading:

As of uuid@7.x this library now provides ECMAScript modules builds,
which allow packagers like Webpack and Rollup to do "tree-shaking" to
remove dead code.

* Bump y18n from 3.2.1 to 3.2.2

Bumps [y18n](https://github.com/yargs/y18n) from 3.2.1 to 3.2.2.
- [Release notes](https://github.com/yargs/y18n/releases)
- [Changelog](https://github.com/yargs/y18n/blob/master/CHANGELOG.md)
- [Commits](https://github.com/yargs/y18n/commits)

Signed-off-by: dependabot[bot] <support@github.com>

* flushing when payload max size has reached

* Bump handlebars from 4.7.6 to 4.7.7

Bumps [handlebars](https://github.com/wycats/handlebars.js) from 4.7.6 to 4.7.7.
- [Release notes](https://github.com/wycats/handlebars.js/releases)
- [Changelog](https://github.com/handlebars-lang/handlebars.js/blob/master/release-notes.md)
- [Commits](handlebars-lang/handlebars.js@v4.7.6...v4.7.7)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump lodash from 4.17.20 to 4.17.21

Bumps [lodash](https://github.com/lodash/lodash) from 4.17.20 to 4.17.21.
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.17.20...4.17.21)

Signed-off-by: dependabot[bot] <support@github.com>

* Bump hosted-git-info from 2.7.1 to 2.8.9

Bumps [hosted-git-info](https://github.com/npm/hosted-git-info) from 2.7.1 to 2.8.9.
- [Release notes](https://github.com/npm/hosted-git-info/releases)
- [Changelog](https://github.com/npm/hosted-git-info/blob/v2.8.9/CHANGELOG.md)
- [Commits](npm/hosted-git-info@v2.7.1...v2.8.9)

Signed-off-by: dependabot[bot] <support@github.com>

* bumping up payload size to 500kb

* lock

* returning promise from flush

* sample code to test more easily

* making queue big enough that it would reach queue size limit

* dropping support for node 8

* segmentio#284 Added options for axiosRetryConfig, disable axiosRetry if retryCount is 0

* Remove unnessesary `|| {}`

Co-authored-by: Patrick Bassut <patrickbassut@hotmail.com>

* prep for 5.0.0 release

* v5.0.0

* Bump tar from 4.4.10 to 4.4.15

Bumps [tar](https://github.com/npm/node-tar) from 4.4.10 to 4.4.15.
- [Release notes](https://github.com/npm/node-tar/releases)
- [Changelog](https://github.com/npm/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v4.4.10...v4.4.15)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump path-parse from 1.0.6 to 1.0.7

Bumps [path-parse](https://github.com/jbgutierrez/path-parse) from 1.0.6 to 1.0.7.
- [Release notes](https://github.com/jbgutierrez/path-parse/releases)
- [Commits](https://github.com/jbgutierrez/path-parse/commits/v1.0.7)

---
updated-dependencies:
- dependency-name: path-parse
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Update test.js

Co-authored-by: Patrick Bassut <patrickbassut@hotmail.com>

* Bump jszip from 3.2.1 to 3.7.1

Bumps [jszip](https://github.com/Stuk/jszip) from 3.2.1 to 3.7.1.
- [Release notes](https://github.com/Stuk/jszip/releases)
- [Changelog](https://github.com/Stuk/jszip/blob/master/CHANGES.md)
- [Commits](Stuk/jszip@v3.2.1...v3.7.1)

---
updated-dependencies:
- dependency-name: jszip
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump tar from 4.4.15 to 4.4.19

Bumps [tar](https://github.com/npm/node-tar) from 4.4.15 to 4.4.19.
- [Release notes](https://github.com/npm/node-tar/releases)
- [Changelog](https://github.com/npm/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v4.4.15...v4.4.19)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* segmentio#287 Flush now returns promises correctly with the data that callback would have gotten. Removed pify from devDependencies

* Bumped axios version to patch ReDos vulnerability

* update history + bump package.json

* let np takes care of version

* v5.1.0

* update axios retry

* version bump with np

* v5.1.1

* pinning axios-retry to 3.2.0

* v5.1.2

* flush when approaching the limit instead of surpassing the limit

* v5.2.0

* v6.0.0

* callback called twice fixed

* enqueue's flushes await

* undo wrong commit

* donde function updated

* Test fixed

* Update code snippet to match analyics-code API

Following the [documentation example here](https://segment.com/docs/connections/sources/catalog/libraries/server/node/#track) as well as the official `analytics-node` API signature, this code snippet example needs to be updated. The current example is from `analytics.js`, Segment's clientside library.

* Bump ajv from 6.10.0 to 6.12.6

Bumps [ajv](https://github.com/ajv-validator/ajv) from 6.10.0 to 6.12.6.
- [Release notes](https://github.com/ajv-validator/ajv/releases)
- [Commits](ajv-validator/ajv@v6.10.0...v6.12.6)

---
updated-dependencies:
- dependency-name: ajv
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump follow-redirects from 1.14.3 to 1.14.8

Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.14.3 to 1.14.8.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.14.3...v1.14.8)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump trim-off-newlines from 1.0.1 to 1.0.3

Bumps [trim-off-newlines](https://github.com/stevemao/trim-off-newlines) from 1.0.1 to 1.0.3.
- [Release notes](https://github.com/stevemao/trim-off-newlines/releases)
- [Commits](stevemao/trim-off-newlines@v1.0.1...v1.0.3)

---
updated-dependencies:
- dependency-name: trim-off-newlines
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump minimist from 1.2.5 to 1.2.6

Bumps [minimist](https://github.com/substack/minimist) from 1.2.5 to 1.2.6.
- [Release notes](https://github.com/substack/minimist/releases)
- [Commits](https://github.com/substack/minimist/compare/1.2.5...1.2.6)

---
updated-dependencies:
- dependency-name: minimist
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump node-fetch from 2.6.1 to 2.6.7

Bumps [node-fetch](https://github.com/node-fetch/node-fetch) from 2.6.1 to 2.6.7.
- [Release notes](https://github.com/node-fetch/node-fetch/releases)
- [Commits](node-fetch/node-fetch@v2.6.1...v2.6.7)

---
updated-dependencies:
- dependency-name: node-fetch
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* Update library axios to 0.27.2

* Add sentat note to readme

Resolves Issue segmentio#338

* add an errorHandler property to the class initializer such that we can return a value using this error handler in case of axios exception instead of throwing. unit test included.

* Update History.md

* Update History.md

* Update History.md file with new changes

* Update History.md file to 6.1.0

* Update History.md

* v6.1.0

* Ensure callback is called when envoking errorHanlder

A property, errorHanlder, was recently added which will be called
instead of throwing an error in the .flush() method. This is important
because errors in .flush() could, previously, only be handled via
process.on('uncaughtException', err => { ... }).

However, this property is currently unusable as, when the flush method
invokes this property, it fails to call the callbacks of the events
being flushed.

This commit makes sure the callbacks are called.

* v6.2.0

* flush: ensure previous flush completion

* allow recovering from failed flushs

* standard --fix

* fix error handling

* first check for reasons not to flush and then for pending flushes

* feat: added gzip

* test: debugging test cases

* test: fix issues with gzip and 404 responses of dataplane batch endpoint

* chore: turned bull into optional dependency, queue prefix logic updated

* chore: removed unnecessary files

* chore: added dataplaneurl support in optional config

* chore: updated bull package dependency to latest

* ci: added github actions

* chore: eslintcache ignored

* ci: added nvmrc

* ci: updated test workflow

* chore: added example apps, test cases updated

* test: test cases updated

* ci: enabled sonar

* chore: type declaration added for screen API

* chore: added .env for example apps, version bumped

* chore: minor changes

* chore: security vulnerabilities adressed

* chore: cli app updated

* ci: sonar project properties update

* chore: changelog updated

* chore: readme update

* chore: readme update

* chore: readme update

* chore: readme update

* chore: cleanup, changelog update

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Christopher Pardy <chris@rategravity.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Albertinin Mourato <ams11@cin.ufpe.br>
Co-authored-by: Emanuele Libralato <10ko@users.noreply.github.com>
Co-authored-by: Kris Dages <krisdages@git.whiteboxsoftware.net>
Co-authored-by: Pooya Jaferian <pooya.j@gmail.com>
Co-authored-by: ankur <ankurchandraa@gmail.com>
Co-authored-by: David Reichert <dreichert@gmail.com>
Co-authored-by: albertmourato <albertinin.mourato@liferay.com>
Co-authored-by: Shane L. Duvall <shane@northtwofive.com>
Co-authored-by: Patrick Bassut <patrickbassut@hotmail.com>
Co-authored-by: Garrett Wu <wugarrett@gmail.com>
Co-authored-by: Dave <dhaden@atlassian.com>
Co-authored-by: Dahaden <dahaden@gmail.com>
Co-authored-by: Shane L. Duvall <sduvall@mark4tech.com>
Co-authored-by: Felipe Najson <felipe@team.northtwofive.com>
Co-authored-by: Felipe Najson <89416739+felipe-najson-ntf@users.noreply.github.com>
Co-authored-by: Benjamin Hoffman <6520022+benjaminhoffman@users.noreply.github.com>
Co-authored-by: “Edson <edson@alcatrazstudios.com>
Co-authored-by: Tam CARRE <kokone@takat.su>
Co-authored-by: Tim Haley <thaley@atlassian.com>
Co-authored-by: Horacio Peña <horape@gmail.com>
Co-authored-by: Michael Grosse Huelsewiesche <mihuelsewiesche@twilio.com>
Co-authored-by: George Bardis <gbardis@rudderstack.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
2 participants