search

ipfs-shipyard / ipfs-pubsub-1on1

1-to-1 communication channel over IPFS Pubsub between two peers
25 stars 11 forks source link

Windows connection #7

Closed BartKnucle closed 5 years ago

BartKnucle commented 5 years ago

Path.join is plateform dependant. Resolve the connections issues from windows

BartKnucle commented 5 years ago

Changed the branche to windows for more clarity

RichardLitt commented 5 years ago

Thanks @BartKnucle. I tried to get this working on Windows, but I'm afraid that Windows is out of my ken, and it took me thirty minutes of work installing various programs to realize that even if I did get your PR downloaded on my machine, I'm not entirely sure that I could adequately test your PR.

So, to try another tack; Looking at the path documentation, Node should automatically take into account Windows paths. See here. Do you know why this PR is necessary, given that?

BartKnucle commented 5 years ago

Hello @RichardLitt,

The problem with path.join under my setup (Win10) is that it construct the path with backslash. Ex:

"\ipfs-pubsub-direct-channel\v1\QmUFSH8QBmwbrnEGtz8Sp5pqVKCYD76MPwizr2r443KxkN\QmbbL2t5ojqQTXtkAWarLdK3WaNdVWvpVMWsop4MfV1jkY"

But under linux or in the browser, the construction is with forward slash Ex:

/ipfs-pubsub-direct-channel/v1/QmUFSH8QBmwbrnEGtz8Sp5pqVKCYD76MPwizr2r443KxkN/QmbbL2t5ojqQTXtkAWarLdK3WaNdVWvpVMWsop4MfV1jkY

For the pubsub 1on1, it implies that the browser can't reach the win10 channel.

And more globaly on orbitdb, since the database hash construction is dependent on the path, for the sames database parameters entry we have different database hash.

I think we should have a call to move forward on the windows implementation. The modified source code works very well on my windows setup and it can communicate also well with the linux and browser setup.

BartKnucle commented 5 years ago

To go a little further, I tested the path.normalize() function, same problem and also tried to set path.sep='/' manually before the path.join(), I also have the same issue. I really think we should avoid the path.join and use the array join function to construct multiplatform paths.

haadcode commented 5 years ago

Thanks @BartKnucle, sounds like a good approach 👍

haadcode commented 5 years ago

FYI, fixed the code so that the / between protocol version and the peer ID remains, fixed it in this commit to master https://github.com/ipfs-shipyard/ipfs-pubsub-1on1/commit/1671eeee18a33b3afda09fadc9044ab489cc1e4e#diff-d17c447bfb7dec05f2c5e09cd23487f9R76. Publishing to npm as 0.0.4.

haadcode commented 5 years ago

Sorry not 0.0.04, published as 0.0.5

BartKnucle commented 5 years ago

this._id` = '/' + PROTOCOL + '/' + this._peers.join('/') is my latest version, remove the unnecesseray ''

haadcode commented 5 years ago

Removed in https://github.com/ipfs-shipyard/ipfs-pubsub-1on1/commit/40dd26986b6a94a9ca90fbaac797b4cfe6153e54. Thanks @BartKnucle, good catch 👍

RichardLitt commented 5 years ago

Ah... I get it now. Thanks, @BartKnucle!