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

fix: shutdownAsync flushes the whole queue #191

Merged
merged 16 commits into from
Feb 26, 2024
Merged

Conversation

marandaneto
Copy link
Member

@marandaneto marandaneto commented Feb 26, 2024

Problem

If the queue is bigger than flushAt after all pendingPromises are resolved, some events can still be lost since flush caps to flushAt anyway.
Discussed here with @benjackwhite #177 (comment)

Changes

Instead of calling shutdownAsync twice, we call until the queue is empty.

Release info Sub-libraries affected

Bump level

  • Major
  • Minor
  • Patch

Libraries affected

  • All of them
  • posthog-web
  • posthog-node
  • posthog-react-native

Changelog notes

  • Added support for X

Copy link

github-actions bot commented Feb 26, 2024

Size Change: 0 B 🆕

Total Size: 0 B

compressed-size-action

@marandaneto marandaneto changed the title chore: release rn 2.11.6 fix: shutdownAsync flushes the whole queue Feb 26, 2024
// If we've been waiting for more than the shutdownTimeout, stop it
const now = Date.now()
if (startTime + timeout >= now) {
break
Copy link
Member Author

Choose a reason for hiding this comment

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

will silently break instead of throwing an error or something since its an intended behavior.

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Other than a nit - love it!

posthog-core/src/index.ts Outdated Show resolved Hide resolved
posthog-core/src/index.ts Outdated Show resolved Hide resolved
@@ -43,14 +43,14 @@
"eslint-plugin-react": "^7.30.1",
"eslint-plugin-react-hooks": "^4.6.0",
"husky": "^8.0.1",
"jest": "^28.1.3",
"jest": "^29.0.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

Relates to jestjs/jest#14741 (comment) otherwise I cannot run tests locally

@marandaneto
Copy link
Member Author

marandaneto commented Feb 26, 2024

@benjackwhite there's just one test failing but its unrelated to my changes I guess.

FAIL posthog-node/test/feature-flags.spec.ts
● match properties › with relative date operators: is_date_before, 1h, 2022-05-01 00:00:00

expect(received).toBe(expected) // Object.is equality

Expected: false
Received: true

  2040 |   ])('with relative date operators: %s, %s, %s', (operator, value, date, expectation) => {
  2041 |     jest.setSystemTime(new Date('2022-05-01'))
> 2042 |     expect(matchProperty({ key: 'key', value, operator }, { key: date })).toBe(expectation)
       |                                                                           ^
  2043 |
  2044 |     return
  2045 |   })

  at posthog-node/test/feature-flags.spec.ts:2042:75

Could it be because of #189 (comment) ?

edit: it was probably flaky since its green now

@marandaneto marandaneto marked this pull request as ready for review February 26, 2024 15:35
@marandaneto marandaneto merged commit 91c6833 into main Feb 26, 2024
4 checks passed
@marandaneto marandaneto deleted the fix/shutdown-flush-all branch February 26, 2024 15:42
@neilkakkar
Copy link
Contributor

oh indeed, much better, thank you!

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