chore: make hasBinary of socket.io-parser work more like the original

[sainthkh]

• Related with #16641 comments

User facing changelog

N/A

Additional details

• Why was this change necessary? => The patched hasBinary should work like the original

Any implementation details to explain?

The original hasBinary detects binary data inside toJSON() functions.

But the patched version doesn't do that because we don't have the toJSON argument. The obj.toJSON && typeof obj.toJSON === "function" && arguments.length === 1 in the code is always false because arguments.length is always true.

This PR solves this problem.

What is affected by this change?

I guess there are none of them. Even if binary objects are In toJSON(), it will be converted to string eventually when encoding them. So, the encoded result is the same.

It might be useful when a developer use the patched hasBinary directly in their code. But it doesn't change anything for now.

How has the user experience changed?

N/A

PR Tasks

• Have tests been added/updated?

[cypress-bot]

Thanks for taking the time to open a PR!

• Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
• Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

[sainthkh]

flaky failures.

[flotwig]

The original hasBinary detects binary data inside toJSON() functions.
But the patched version doesn't do that because we don't have the toJSON argument.

In the linked code, the toJSON parameter is unused inside of the function. Why is this change that brings the unused parameter back desirable?

[sainthkh]

@flotwig

Yes, it is unused directly. But it affects the flow of the code because of the line 54, arguments.length === 1. Because there are 2 ways to call this functions: hasBinary(obj) and hasBinary(obj, true) like the line 56.

As the second argument of our patched function is an empty array by default, arguments.length is always 2 and the result of obj.toJSON && typeof obj.toJSON === "function" && arguments.length === 1 is always false. It means that our patched function doesn't check if it has binary inside the toJSON method of the obj.

(Actually, it took me a few days to understand this. I think the code is really unintuitive.)

As I said above, it doesn't affect Cypress functions. It's just done to make the patched function work like the original.

I've written a test to showcase this behavior difference.

    it('hasBinary handles binary data in toJSON()', () => {
      const x = {
        toJSON () {
          return Buffer.from('123', 'utf8')
        },
      }

      const data = ['a', x]

      expect(hasBinary(data)).to.be.true // It's true in the original, but false in the patched.
    })

[flotwig]

Thanks for taking the time to explain. I missed the arguments.length on my first and second reads 🤦 LGTM