small fixes

[jimmywarting]

...While trying out webtorrent in Firefox with your code i notice that it didn't work. Do you know why?

[TexKiller]

Nice!
GitHub wasn't emailing me about notifications, so I only saw this now.

I believe on ping.js, line 20 should be just ports[0].postMessage(entry[1], [ entry[0] ]) instead of evt.ports[0].postMessage(entry[1], [ entry[0] ]). Sorry for the e variable mayhem.

By the way, should we not check if the readable is locked when we test for transferable stream compatibility? And is it not possible for a polifilled TransformStream to return a transferable ReadableStream?

I think I'll have time to test the webtorrent example the day after tomorrow.

[jimmywarting]

By the way, should we not check if the readable is locked when we test for transferable stream compatibility?

Not necessary really... it will catch an error when it fails to transfer a stream. and it will always be locked (there is no buggy implementation somewhere where locked are incorrect).

And is it not possible for a polifilled TransformStream to return a transferable ReadableStream?

Like i sad earlier, if you try to pass something to that isn't transferable in the array of transferable it will throw ann error. that includes polyfilled objects, they need to be natively supported to know what a transferable object is.

targetWindow.postMessage(message, targetOrigin, [transferable]);

https://developer.mozilla.org/en-US/docs/Web/API/Transferable

mdn don't list TransformStream but it's new and not listed...

Sorry for the e variable mayhem.

Not so picky about that when folks tries to help with PR. can just fix them the next time i encounter them myself 😉
I'm following StandardJS rules, and don't like sort variables cuz they are annoying to search and replace when using multiple cursors.

[jimmywarting]

I think i see now what the problem was

[jimmywarting]

Fixed the port now.

The problem still persist... still the other examples worked out grate but the webtorrent example
With this pice of code. the problem is not getting the save dialog or the download to happen cuz that happens once it writes any data to the stream

         if (iframe && !loaded) {
            iframe.addEventListener('load', () => {
              window.location = url
            }, { once: true })
          } else {
            window.location = url
          }

firefox tries to navigate away from the page (to the download url - intercept-me#), therefore stopping webtorrent from creating any new webrtc connections resulting with no connection -> no metadata -> nothing to download -> nothing to write -> leading to the save dialog not showing.
When i commented out window.location = url it would start the torrent download and eventually write to the stream

Don't remember if i had this problem before when i used to create a download link and clicking on it...
Was it something bad by using a hidden iframe instead of using window.location = url? cuz that seems to work...

[TexKiller]

Yeah, navigating away from the page with window.location has the downside of closing any open connections, even if the navigation is reverted because of the download. That is why I have waited for the iframe to load there, if it hadn't already (the SharedWorker could still be loading inside the iframe when the download starts on platforms that use it to ping the ServiceWorker).

I guess we can do what you say: remove the option of navigating to the download link, and replace it with the hidden iframe approach. It should work on all contexts that are currently using window.location, I think. It creates extra iframes, but also removes them after the download starts, so it shouldn't matter. You can just remove lines 101 through 109, then remove line 95, and it should work.

When I talked about the polifilled TransformStream, I was thinking that maybe a polifilled TransformStream could provide a native and transferable ReadableStream on its readable property, even though TransformStream itself isn't. Or maybe that is not possible, I have no idea.

[jimmywarting]

Think I would like to have I iframe approach.

Hmm. Never thought about only having transferable readablestream. We have never tried that. Most likely a polyfilled transformer would also return a polyfilled readablestream as well. So we would have to pipe a polyfilled writable to a native readablestream ourselves. That's more complicated. Let's hold back on that for now and do what is being done now. Can dig into it when that scenario happens

[jimmywarting]

There, what do you think now?

Good if someone else could test it also. (seems to work fine in chrome and firefox)

[TexKiller]

Sorry, I forgot to say the "closed connections" situation only happens when navigating away from the top page, it should not affect iframes at all. So your lines 99 through 104 and line 97 are not necessary anymore, as they will do nothing but delay the download and clutter the code. In fact, they are bad because they create iframes that are never removed. Line 98 sets up the iframe to be removed once the download starts.

Also, your change to the secure context detection must have broken Web Extension's background pages on Firefox, since they were purposefully being treated as insecure by me before, despite window.isSecureContext being true. I did that because the flow there is similar to other insecure pages, so it was easier to do that than to try and branch everywhere with ifs.

If you set your isSecureContext after the background page check, you could do:

const isSecureContext = window.isSecureContext && (!firefox || !background)

I still can't dedicate much time to this, the deadline on another project is only a few hours away. I'll do a lot of tests on everything tomorrow, and get the PR up to date. :)