Revert "🤖 Merge PR #49622 [analytics-node] Correct message callback t…

[benjaminhoffman]

…ype by @nicklasmoeller"

This reverts commit 8260410.

For context, see here:

• Segment introduced a second param here: Callback called twice segmentio/analytics-node#315
• I show that it works according to their documentation in my comment here: [analytics-node] Correct message callback type #49622 (comment)

cc @nicklasmoeller

Please fill in this template.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change.
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm test <package to test>.

Select one of these and delete the others:

If adding a new definition:

• The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
• If this is for an npm package, match the name. If not, do not conflict with the name of an npm package.
• Create it with dts-gen --dt, not by basing it on an existing project.
• Represents shape of module/library correctly
• tslint.json should contain { "extends": "@definitelytyped/dtslint/dt.json" }, and no additional rules.
• tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: <>
• If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

If removing a declaration:

• If a package was never on Definitely Typed, you don't need to do anything. (If you wrote a package and provided types, you don't need to register it with us.)
• Delete the package's directory.
• Add it to notNeededPackages.json.

[typescript-bot]

@benjaminhoffman Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

1 package in this PR

• analytics-node — on npm, on unpkg

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

You can test the changes of this PR in the Playground.

Status

• ❌ No merge conflicts
• ✅ Continuous integration tests have passed
• ✅ Most recent commit is approved by type definition owners or DT maintainers

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This PR has been inactive for 31 days — it is considered abandoned, and therefore closed!

---

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 58654,
  "author": "benjaminhoffman",
  "headCommitOid": "efcbfc6cb8fc18484963b6452a7b9af2f35ba0a7",
  "mergeBaseOid": "9302ea3a51bf1308d87353ce51fa2cca7d30d5de",
  "lastPushDate": "2022-02-08T04:22:09.000Z",
  "lastActivityDate": "2022-03-10T04:22:21.000Z",
  "hasMergeConflict": true,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Popular",
  "pkgInfo": [
    {
      "name": "analytics-node",
      "kind": "edit",
      "files": [
        {
          "path": "types/analytics-node/analytics-node-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/analytics-node/index.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "fongandrew",
        "thomasthiebaud"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "RyanCavanaugh",
      "date": "2022-03-10T04:22:21.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 1032202278,
  "ciResult": "pass"
}

[typescript-bot]

🔔 @fongandrew @thomasthiebaud — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

[typescript-bot]

Re-ping @fongandrew, @thomasthiebaud:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

[typescript-bot]

It has been more than two weeks and this PR still has no reviews.

I'll bump it to the DT maintainer queue. Thank you for your patience, @benjaminhoffman.

(Ping @fongandrew, @thomasthiebaud.)

[nicklasmoeller]

Hi @benjaminhoffman. Thanks for tagging me.

As you probably noticed, it landed in segmentio/analytics-node@f5a1385 and still hasn't reached a release yet. However, when it does, this definitely makes sense to get merged in.

Your tests in #49622 (comment) may have been due to the callback from your message methods (e.g. through enqueue) being passed to flush as its own callback. That behavior is also changed on the non-released master branch - the same commit that I referenced before.

You may be seeing this behavior since the first message in the queue would bind its callback to the flush callback in a good handful of scenarios:

• on the first message
• when the max items or queue size has been met
• for the periodic flushing that gets set up when a message is enqueued

Hence, what you're seeing, should be the callback provided to the flush method, through delegation from the enqueue method. You should be able to confirm this, by running three track (or similar) invocations with callbacks, and you'd only see the first two callbacks if you stick to the default configuration, where the first message gets flushed immediately, and then the second message's callback gets bound to the flush callback, that's bound to the timer.

I acknowledge that it doesn't feel super transparent what's going on, but I hope that the explanation clears things up a bit for you.

I'd say that the changes live up to the current situation of the master branch on of analytics-node, given that the callbacks are now being invoked with the queue batch data, as of these lines of code. It's not a tagged release, nor released to npm yet, so I don't know what the practice is with DefinitelyTyped in such situations.

TLDR; This covers unreleased code, but once Segment decides to release their next version after 6.0.0, these changes are 👍

cc maintainers @fongandrew and @thomasthiebaud

[benjaminhoffman]

@nicklasmoeller I agree! ... and came to a similar conclusion after submitting this change. I figured Segment would release soon but I guess not. 🤷

Would also love if there was a type for the flush prop, as seen here in this example.

[RyanCavanaugh]

I'm going to approve and the bot is going to ask you to merge, but I'll ask that you hold off on merging until the upstream change is live. If this PR times out you might end up needing to re-open it, unfortunately - the automation 'round these parts isn't set up for that sort of situation.

[typescript-bot]

Re-ping @benjaminhoffman / @fongandrew, @thomasthiebaud:

This PR has been ready to merge for over a week, and I haven't seen any requests to merge it. I will close it on Apr 9th (in three weeks) if this doesn't happen.

(If there's no reason to avoid merging it, please do so. Otherwise, if it shouldn't be merged or if it needs more time, please close it or turn it into a draft.)

[typescript-bot]

@benjaminhoffman Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day!

[typescript-bot]

@benjaminhoffman I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Apr 9th (in a week) if the issues aren't addressed.

[typescript-bot]

@benjaminhoffman To keep things tidy, we have to close PRs that aren't mergeable and don't have activity in the last month. No worries, though — please open a new PR if you'd like to continue with this change. Thank you!