@cwadrupldijjit 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

• ws — on npm, on unpkg
  • owner-approval: @k-yle
  • 1 added owner: ✎@cwadrupldijjit

@cwadrupldijjit: I see that you have added yourself as an owner, are you sure you want to become an owner?

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

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 a DT maintainer

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

{
  "type": "info",
  "now": "-",
  "pr_number": 58753,
  "author": "cwadrupldijjit",
  "headCommitOid": "954895b186ea07c982148461f3ffbed0e729f5aa",
  "mergeBaseOid": "7dec77e4522d220346c90fcd09044e78feb49dd0",
  "lastPushDate": "2022-02-18T17:49:04.000Z",
  "lastActivityDate": "2022-02-19T02:53:47.000Z",
  "mergeOfferDate": "2022-02-18T23:37:44.000Z",
  "mergeRequestDate": "2022-02-19T02:53:47.000Z",
  "mergeRequestUser": "cwadrupldijjit",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "ws",
      "kind": "edit",
      "files": [
        {
          "path": "types/ws/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/ws/ws-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "loyd",
        "mlamp",
        "TitaneBoy",
        "reduckted",
        "teidesu",
        "wojtkowiak",
        "k-yle"
      ],
      "addedOwners": [
        "cwadrupldijjit"
      ],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "andrewbranch",
      "date": "2022-02-18T23:37:04.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "k-yle",
      "date": "2022-02-18T20:35:11.000Z",
      "isMaintainer": false
    },
    {
      "type": "approved",
      "reviewer": "vansergen",
      "date": "2022-02-18T19:35:12.000Z",
      "isMaintainer": false
    },
    {
      "type": "stale",
      "reviewer": "doroodgar",
      "date": "2022-02-17T12:27:57.000Z",
      "abbrOid": "d2c6c9f"
    }
  ],
  "mainBotCommentID": 1037950088,
  "ciResult": "pass"
}

🔔 @loyd @mlamp @TitaneBoy @reduckted @teidesu @wojtkowiak @k-yle — 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.

Note: The second check through Azure DevOps seems to have failed due to TypeScript not being where it was expected, which doesn't have anything to do with my changes.

Also another note: The dtslint cli was having issues when I ran it either directly or with npm test ws. It goes up a level from the project root to look for the notNeededPackages.json, which naturally doesn't exist there. Running the test all script, it worked correctly. I will have to create an issue in the dtslint repo to ask about it or suggest a change.

I'd think both approaches could work simultaneously. In my case, I don't want to create a custom WebSocket class to use instead. I just would augment the original WebSocket slightly.

To be fair, I could create a new class, but if it's only for adding a single property, that feels like a little much.

But I could also swallow my pride and do it that way instead if that's just how it should be. Interested to hear others weigh in on this to see if it's necessary.

Though thinking back to the other two PRs, the one that looks like it would do closer to what I want would be #58772. The other seems to make sense, too. Those coincide fine.

But I might also note that my approach doesn't change the original class (such as if you import default or import..require) but does change the type found as a namespaced item on that imported object (as far as I can tell).

Example:

// these can't be augmented
import WebSocket = require('ws');
import WebSocket from 'ws';

// these, however, can
import { WebSocket } from 'ws';
import * as wslib from 'ws';
declare const websocket: wslib.WebSocket;

In my case, since I'm never creating a ws object via the class constructor call, importing the type from the package seems to work just fine. It augments everywhere that I've tried it so far.

But I could also swallow my pride and do it that way instead if that's just how it should be.

The discussion in websockets/ws#2007 suggests that both approaches are valid

I'd think both approaches could work simultaneously.

I think you're right, we should go ahead with this PR and #58772, which would make #58555 redundant (unless I'm misunderstanding the benefit of that PR?)

I think the approach in #58555 would be better unless someone wants to make a single pull request that combines this and #58772?

I'd say that the this argument in #58555 makes a little more sense from an implementation standpoint, because in theory, that allows the class-based syntax to work fine, too (as seen in #58772). The only thing that might not be the case is if somehow, the original WebSocket implementation is the only value that comes through instead of the custom class. It also promotes better typings by allowing for the dynamicism of this over just the WebSocket implementation.

The result of mine is more related to adding to the original structure of the WebSocket objects instead of creating a new type to handle it. The this: this reference would still handle it just as well as leaving it as this: WebSocket, I believe, so it doesn't fully negate the use of it.

I'm also personally for more flexibility than anything. I don't think any of those approaches are "wrong", though I think that the use cases for each approach might vary.

Prefer to use your own WebSocket class? That's available for you. Want to fix the type of the this keyword to make more sense? That's also available for you. Just want to augment each time a WebSocket is used to add minimal properties to it? That also works.

Does that sound reasonable to you @k-yle and @lavalleeale?

Since I anticipate there will be merge conflicts between the three for the tests, I'm willing to merge both of your branches into mine, fix the merge conflicts, and update this PR with those changes, if that makes sense and works for you. Or I'm willing to be the last one to deal with those in my code if you'd prefer.

I think that sounds all good to me as long as the tests reflect all of the use cases

Do we really need tests here?🤔 Otherwise LGTM

@cwadrupldijjit The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

Alas... A blank line that broke the CI...

@vansergen, @k-yle, @doroodgar Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

My heart about stopped when I thought that all of the checks failed again, even though I watched the GitHub action run to successful completion...

Sigh of relief when I refreshed the page and saw that it said all checks had passed.

@k-yle, @doroodgar Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

@doroodgar Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

@cwadrupldijjit: Everything looks good here. I am ready to merge this PR (at 954895b) on your behalf whenever you think it's ready.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

(@loyd, @mlamp, @TitaneBoy, @reduckted, @teidesu, @wojtkowiak, @k-yle: you can do this too.)

Ready to merge