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

Check if protocol is defined. #2421

Closed
wants to merge 1 commit into from

Conversation

passabilities
Copy link

@passabilities passabilities commented Feb 25, 2019

Description

Without this check, if an undefined value is passed within Firefox, it fails to connect.

Fixes #1965

Type of change

  • Bug fix
  • New feature
  • Breaking change

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no warnings.
  • I have updated or added types for all modules I've changed
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run test in the root folder with success and extended the tests if necessary.
  • I ran npm run build in the root folder and tested it in the browser and with node.
  • I ran npm run dtslint in the root folder and tested that all my types are correct
  • I have tested my code on the live network.

Without this check, if an undefined value is passed within Firefox, it fails to connect.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 93.073% when pulling e95196b on TotlePlatform:fix/null-protocol into 03b2d39 on ethereum:1.0.

@nivida
Copy link
Contributor

nivida commented Feb 26, 2019

First of all, thanks for opening this PR! I just checked the WebSocket documentation.

Either a single protocol string or an array of protocol strings. These strings are used to indicate sub-protocols, so that a single server can implement multiple WebSocket sub-protocols (for example, you might want one server to be able to handle different types of interactions depending on the specified protocol). If you don't specify a protocol string, an empty string is assumed.

This means if nothing got passed e.g.: undefined it should use a empty string. I will test it but I think this would be the better solution:

if (typeof options.protocol === 'undefined') {
  options.protocol = '';
}
connection = new window.WebSocket(url, options.protocol);

Can you tell me with which version of FF this issue has? latest?

@passabilities
Copy link
Author

Yes, this is happening on the latest FF 65.0.1. It's especially weird because After doing some manual debugging, it appears to actually be defaulting to an empty string when it is undefined but still throws this error. However, when testing by not passing anything in the second argument it works perfectly. I will test if it works with an empty string.

@nivida nivida added On Ice Important but no longer pursued for the near future and removed more information needed labels Feb 27, 2019
@nivida
Copy link
Contributor

nivida commented Apr 17, 2019

@passabilities Did you find the time to test it?:)

@nivida nivida closed this Jun 12, 2019
@nivida
Copy link
Contributor

nivida commented Jun 12, 2019

This PR got closed because of the reorganisation of the branches. Feel free to open a new PR for the 2.x branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On Ice Important but no longer pursued for the near future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants