Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

Awesome WebWorkers Support #725

Closed
7 tasks done
dignifiedquire opened this issue Jan 26, 2017 · 42 comments
Closed
7 tasks done

Awesome WebWorkers Support #725

dignifiedquire opened this issue Jan 26, 2017 · 42 comments
Assignees
Labels
awesome endeavour help wanted Seeking public contribution on this issue

Comments

@dignifiedquire
Copy link
Member

dignifiedquire commented Jan 26, 2017

Thanks to the efforts from @dryajov and @tswindell we are close to having full support for running js-ipfs in webworkers. This issue tracks the last outstanding issues for this.

Aegir

  • Merge webworkers support into aegir
  • Release aegir with webworker test support

PRs

Others

  • Enable webworker tests for all js-ipfs* repos
  • Enable webworker tests for all js-libp2p* repos
  • Enable webworker tests for all js-multi* repos
@dryajov
Copy link
Member

dryajov commented Jan 27, 2017

@dignifiedquire @diasdavid I can take this on:

Enable webworker tests for all js-ipfs* repos
Enable webworker tests for all js-libp2p* repos
Enable webworker tests for all js-multi* repos

This is required to move forward with #127

@daviddias
Copy link
Member

daviddias commented Jan 29, 2017

This is awesome! Thank you @dryajov!

Can we then make a lot of noise sure everyones voice is represented on the request to have WebRTC in WebWorkers?

@daviddias daviddias added the status/deferred Conscious decision to pause or backlog label Jan 29, 2017
@dryajov
Copy link
Member

dryajov commented Feb 1, 2017

Wanted to check in with where things are with this endeavor.

So far, I've managed to switch the use of window to self, and this seem to fix 90% of the issues. I've got most of it committed in my forks on github, if anyone is interested to check it out, please do. I'm not entirely sure about using self being the right way of doing it, and I'm not as familiar with webpack to know if there is a better approach (i'd rather a bundler taking care of context/env), but so far this has worked. @diasdavid @dignifiedquire @kumavis If you guys know of a better way please let me know; otherwise, I think we have a best practice suggestion to make to the community at large - ditch window in favor of self. 👅

So far, the biggest issue I ran into is with brorand, which is not assuming the webworker env correctly. I've got a PR coming that addresses it, and that fixes most of the issues with multihashes. But I'm not sure when thats going to be merged in, I might need help creating noise around it.

I will be creating PRs for each project shortly (today/tomorrow), so that everypbody can keep an eye on the progress and contribute where they see fit.

The rest of the issues are less trivial, and I will be going over them one by one and fixing as I go.

I'll keep everyone posted on progress.

Cheers!

@dryajov
Copy link
Member

dryajov commented Feb 2, 2017

PR to address brorand - indutny/brorand#7

@kumavis
Copy link
Contributor

kumavis commented Feb 2, 2017

i bet you could polyfill it

if (typeof window === 'undefined') self.window = self

@dryajov
Copy link
Member

dryajov commented Feb 2, 2017

wouldn't that mess things up a bit inside a WW for other packages as well? As in incorrectly assuming that its running in a dom enabled env? Otherwise, thats a better approach.

@kumavis
Copy link
Contributor

kumavis commented Feb 2, 2017

yeah thats a good point, its a bit hacky so beware

@dryajov
Copy link
Member

dryajov commented Feb 8, 2017

The linked PRs should address the full webworker support, with this changes I'm able to run js-ipfs tests in webworkers.

@dignifiedquire
Copy link
Member Author

Looks like brorand shipped yesterday with the fixes :)

@daviddias
Copy link
Member

woooot! So now, we just need to enable webworker tests in all the repos? Let's make all of this on time for #756 \o/

@dignifiedquire
Copy link
Member Author

@diasdavid all you have to do for that is upgrade to aegir@10 which runs them by default

@daviddias
Copy link
Member

@dryajov
Copy link
Member

dryajov commented Feb 8, 2017

@diasdavid for projects that we want to skip webworker tests you can pass the --dom parameter to aegir.

@dryajov
Copy link
Member

dryajov commented Feb 8, 2017

@diasdavid I would also merge and release this PRs before bumping aegir on the rest of the projects, because they would fail without those changes, I've seen multihashing-async being the culprit in most cases...

@daviddias
Copy link
Member

why --dom? That doesn't seem like to be something obvious for what it does

@dryajov
Copy link
Member

dryajov commented Feb 8, 2017

there are two flags now, --webworker which will run webworker only tests and --dom which will run tests in a dom enabled env™ only, I've no attachment to that flag whatsoever and open to hearing suggestions - naming is hard. 😛

@daviddias
Copy link
Member

indeed :) let's do --webworker=only and --webworker=false for only webworker and except webworker respectively, being --webworker=true the default

@dryajov
Copy link
Member

dryajov commented Mar 13, 2017

@diasdavid what project is that? This might be a project that needs WW disabled. As for them not failing, are you on the latest Aegir with those fixes applied?

@daviddias
Copy link
Member

This is js-ipfs itself

@dryajov
Copy link
Member

dryajov commented Mar 13, 2017

Ah, thats weird. I'll take a look at it, wonder if any changes have been introduced which could bring window in again? In any case will take a look ASAP.

@daviddias
Copy link
Member

@dryajov still seeing this one, any developments?

@daviddias
Copy link
Member

daviddias commented Mar 16, 2017

@dryajov
Copy link
Member

dryajov commented Mar 16, 2017

@diasdavid, we also need this ipfs/aegir#107 in aegir. For now, we can just disable window as a global without any specific message, if you have a chance to update aegirs eslint with the rule, it might be a good time.

@dryajov
Copy link
Member

dryajov commented Mar 17, 2017

@diasdavid webrtcsupport lib is the one causing issues, it is the last lib we need fixed before full WW support is finally ready. Here is a PR addressing it HenrikJoreteg/webrtcsupport#30.

@fippo
Copy link

fippo commented Mar 18, 2017

@dryajov: i'd advise against using the webrtcsupport package. Its legacy and contains 90% stuff that you don't need anymore in 2017.

@diasdavid i would strongly advise against "making noise", in particular making the kind of noise that has happened on the worker topic in the past.. Trying to get someone else to do work that benefits you like this typically doesn't work out. If you want to be constructive you work together with the people who are able to do that work or do the work (including specifying how things should work) yourself.

@dryajov
Copy link
Member

dryajov commented Mar 18, 2017

@fippo thanks for pointing this out. Indeed all we use it for is detection of the presence of webrtc support in the environment, perhaps don't need that in the browser anymore, but what about nodejs?

@daviddias
Copy link
Member

@fippo I clearly didn't word that in the best way possible. Entirely agree with you, it is orders of magnitude more valuable to provide constructive help and willingness to be part of the process than a simple 'I want this too'. Currently, I'm underwater and unavailable to provide significant help to that development, however, I do hope in the best way possible, that by bringing other people that could also benefit from it, we can signal that there is value on making it happen.

re: webrtcsupport - What is your favourite nowadays?

@fippo
Copy link

fippo commented Mar 20, 2017

@diasdavid at least you didn't tweet about it like certain other people. If you just want to check for support RTCPeerConnection && ('createDataChannel' in RTCPeerConnection.prototype) should be sufficient.

@dryajov
Copy link
Member

dryajov commented Mar 20, 2017

@diasdavid I'll do what @fippo is suggesting.

@kumavis
Copy link
Contributor

kumavis commented Apr 4, 2017

@dignifiedquire found this recently https://github.com/Joris-van-der-Wel/karma-mocha-webworker
may be of interest

@dryajov
Copy link
Member

dryajov commented Apr 4, 2017

@kumavis thats what we ended up using.

To get the last mile of this, we need a pretty small change in how we detect WebRTC. Looking up that issue...

ah... its right in this thread:

#725 (comment)

@daviddias daviddias added status/ready Ready to be worked and removed status/deferred Conscious decision to pause or backlog labels Apr 5, 2017
@daviddias daviddias changed the title Full support for webworkers Awesome WebWorkers Support Apr 14, 2017
@IamCarbonMan
Copy link

As an outside observer, I have to ask: what does all this mean? Are you actually able to use WebRTC APIs in worker code as has been proposed as standard here? Or is this issue just to test that the code should theoretically work the same from a worker context, to be ready when worker WebRTC is standardized?

@dryajov
Copy link
Member

dryajov commented May 8, 2017

Good question, glad you ask - this effort is to get IPFS to run in Web and Service workers. The specific outstanding issue right now is related to WebRTC detection functionality that breaks inside Workers because of explicit usage of window. WebRTC is tangential to this effort, since its not supported in web/service workers at all at this point.

@daviddias
Copy link
Member

@IamCarbonMan you are not able to, but libp2p dark magic uses whatever the runtime has to offer it, it won't use WebRTC if there is no support but the issue is that we are checking the support by checking the window object which is also not defined.

@daviddias
Copy link
Member

WebWorker tests are now passing! :D woot!

#857

@daviddias
Copy link
Member

fully supported in v0.24.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awesome endeavour help wanted Seeking public contribution on this issue
Projects
None yet
Development

No branches or pull requests

6 participants