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

Webworker support #756

Merged
merged 5 commits into from
May 26, 2021
Merged

Webworker support #756

merged 5 commits into from
May 26, 2021

Conversation

owenpearson
Copy link
Member

Webworker support is currently broken, a regression from the move to webpack. This PR adds a new webpack bundle which specifically supports running in a webworker context.

@owenpearson owenpearson self-assigned this May 14, 2021
@ben-xD
Copy link

ben-xD commented May 15, 2021

Oh, but 1 question: would this prevent a regression in the future? I made an environment here which crashed with the window error. Im not sure how we can run tests in a webworker environment though. Otherwise it might be easy to reintroduce this issue again

@owenpearson
Copy link
Member Author

@ben-xD Yeah this should be fairly regression-proof - I can't think of an obvious way to break it, although the webpack 'webworker' target isn't well documented so it's hard to say for certain. I actually tested against your ably-webworker repo when I was developing this so it should work, have you checked that you're using the browser/static/ably-webworker.min.js bundle?

@ben-xD
Copy link

ben-xD commented May 16, 2021

Okay cool. Im not facing any issues anymore because I've switched to Firebase functions which runs in a nodeJS environment. The way I phrased my last sentence makes it sound like I tried your fix, but I didn't because I've moved to Firebase functions. I was just mentioning my observations of the crash before your fix, I wrote that sentence badly, sorry.

But I've tested it now, so I've observed a few things:

  • Resolved (the repo code was updated to use .Promise): Tokens returned by Ably are undefined. that might be because I npm run build't it locally? Or maybe something else is wrong.
  • Typescript warnings for ably-webworker or ably-webworker.min:

Screenshot 2021-05-16 at 14 42 52

Unrelated issue: .Request is not a function

There was the unrelated issue that I faced previously and we spoke about over a video call, happening in Cloudflare Workers:
screenshot_2021-05-14_at_11 10 58

It seemed to us that .Request isn't actually implemented/ defined in one of the files at all, so it makes sense this error is thrown. Owen discovered that Http.Request wasn't actually implement, see line 136 of browser/lib/util/https.js. However, why it only happens in Cloudflare Workers (and not Web Workers in general), I don't know. I personally am not blocked by this bug/ issue because I stopped using Cloudflare Workers because the debugging experience was not good. Firebase functions can be debugged with breakpoints in WebStorm, and it's been great. Though it might be good to support users who wish to use Ably on Cloudflare Workers.

A small bit of detail about Cloudflare Workers: It configures webpack target to be webworker, but it also uses the Service Worker APIs to incept network requests/ act as a server/ serverless function. The reason they do this instead of running NodeJS is cost, performance and scalability. There is no cold start issue like normal serverless, much lower memory footprint. More about that here. So perhaps it is because we do not support running in "service worker", though this is just a guess.

Let me know if you want a separate issue for this.

@owenpearson
Copy link
Member Author

owenpearson commented May 16, 2021

Thanks, I'm aware of the typings issue and need to investigate what the current best approach is. At the moment we have a separate type definitions file for each module you can import (see callbacks.d.ts, promises.d.ts) but that obviously doesn't scale nicely. Can you give me instructions on how to reproduce the undefined tokens behaviour?

And yes, the .Request thing is unrelated to this AFAIK so should be a separate issue. I'm happy for you to open the issue if you don't mind :) please include some basic instructions on how to reproduce. FYI, the reason it looks like .Request is not implemented is because the module is actually mutated elsewhere - if you search around you should see there are a couple of places that set the .Request property on Http (obviously this isn't ideal and should no longer be the case once we've converted http stuff to typescript).

@ben-xD
Copy link

ben-xD commented May 16, 2021

To reproduce,

  • git clone https://github.com/ben-xD/ably-webworkers. If you cloned in the past, ensure you're up to date
  • To ensure this cloned repo uses your branch of Ably, In ably-js directory, run npx link and in ably-webworkers directory, install dependencies npm i, then run npx link ably. I've already updated the code to import ably-webworker.min, see https://github.com/ben-xD/ably-webworkers/blob/e6ae8d2a736da6e2ad36548c7d9c50919d4cc9e7/src/auth.ts#L3
  • Follow the steps in 'getting started' in the readme (npm i, update api key, and npm start)
  • Open the browser, notice in the browser console that the token is undefined.
  • Click webworkers button in the website, and notice the next console.log output for token is also undefined.

Cool, I will make a github issue for the other issue

@jcapogna
Copy link

jcapogna commented May 18, 2021

I'm currently adding Ably to a Cloudflare worker and running into think is this issue:

Error: Something went wrong! Status: 400 Bad Request, Details {
  "result": null,
  "success": false,
  "errors": [
    {
      "code": 10021,
      "message": "Uncaught ReferenceError: window is not defined\n  at line 34\n  at line 34\n  at line 1 in n\n  at line 22\n  at line 1 in n\n  at line 22\n  at line 1 in n\n  at line 22\n  at line 1 in n\n  at line 22\n"
    }
  ],
  "messages": []
}

Is there an old version of Ably that does work in Cloudflare Workers?

@ben-xD
Copy link

ben-xD commented May 18, 2021

Hey @jcapogna,

@owenpearson will know more

@owenpearson
Copy link
Member Author

Hi @jcapogna, thanks for reporting this to us. As Ben said above 1.2.3 should work fine - we have a couple of issues to resolve with this PR but we should have a new version which works with Cloudflare Workers released soon.

@jcapogna
Copy link

Thank you, downgrading to 1.2.3 helped. I'm now having what I believe to be a different problem w/ Ably & Cloudflare so I opened #766

@ben-xD
Copy link

ben-xD commented May 26, 2021

Were we going to add typescript support for this one? There is no autocomplete when using the webworker import, so all the usages everything is a red squiggly line. When I was using it, I commented out the webworker import and used the normal import during development/ coding, and uncommented to build it.

Having said that, if typescript support is coming through other means it's probably not a big deal to wait for that.

@owenpearson
Copy link
Member Author

@ben-xD This should be fixed by 406a689, have you tried it out since that commit?

@ben-xD
Copy link

ben-xD commented May 26, 2021

Oops I read the commit log upside down 🙃, I've tested it now, and it works. Thanks!

@owenpearson owenpearson linked an issue May 26, 2021 that may be closed by this pull request
@owenpearson owenpearson merged commit 65e2917 into main May 26, 2021
@owenpearson owenpearson deleted the feature/webworker-support branch May 26, 2021 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Library apparently broken in webworkers
4 participants