Skip to content

Log errors in CoapClient.tryToConnect()#30

Merged
AlCalzone merged 1 commit intoAlCalzone:masterfrom
oscarnyl:patch-1
Feb 13, 2018
Merged

Log errors in CoapClient.tryToConnect()#30
AlCalzone merged 1 commit intoAlCalzone:masterfrom
oscarnyl:patch-1

Conversation

@oscarnyl
Copy link
Copy Markdown
Contributor

I'm trying to use a library which uses this library (node-tradfri-client), where I've managed to find out that the attempts to establish a connection fail in this method. This commits adds a debug call to attempt to give the user some context as to why the attempt to establish a connection fails.

I'm trying to use a library which uses this library (node-tradfri-client), where I've managed to find out that the attempts to establish a connection fail in this method. This commits adds a debug call to attempt to give the user some context as to why the attempt to establish a connection fails.
@oscarnyl
Copy link
Copy Markdown
Contributor Author

After working around the problem I learned that the problem was that I had upper case characters in the hostname.

We might want to consider any of the following:

  • Normalize the hostname to lower case characters
  • Throw an error if upper case characters is passed as hostname
  • Another creative solution to this mismatch occurring when using the package 'url' to parse out the hostname

@AlCalzone AlCalzone merged commit 79a92a2 into AlCalzone:master Feb 13, 2018
@AlCalzone
Copy link
Copy Markdown
Owner

Thanks! I'm not sure what you mean with your 3rd proposal, can you elaborate please?

AlCalzone added a commit that referenced this pull request Feb 13, 2018
* test DeferredPromise
* build the change from #30
@AlCalzone
Copy link
Copy Markdown
Owner

Oh I think I understand. The given resource URL gets parsed by the url package, which normalizes the hostname somehow. The given hostname for setSecurityParameters does not.

AlCalzone added a commit that referenced this pull request Feb 13, 2018
…en to `tryToConnect` (fixes the issue mentioned in #30)

* added tests for `getConnection`
@AlCalzone
Copy link
Copy Markdown
Owner

AlCalzone commented Feb 13, 2018

Should be fixed in v0.5.4, please test!

@oscarnyl
Copy link
Copy Markdown
Contributor Author

I haven't had the chance to test this but looking at the code it looks like it should solve it completely 👌 Thanks for the quick feedback!

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.

2 participants