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

Pr/make web worker friendly #8

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Almar
Copy link

@Almar Almar commented May 28, 2021

With this change fetchEventSource can be used from within a web worker

@ghost
Copy link

ghost commented May 28, 2021

CLA assistant check
All CLA requirements met.

@romain-guillot-symphony
Copy link

Hello, is there any blockers to merge this PR?

@arthyn
Copy link

arthyn commented Jul 29, 2021

I'd also like this PR to be merged, this would be a huge help, thanks!

@arthyn
Copy link

arthyn commented Dec 9, 2021

@vishwam hey just bumping this again, if you have a chance to look at it, thanks!

@mbiegert
Copy link

I'm also very interested in seeing this PR getting merged. In regards to the other open PRs, is this project still in use at microsoft/ Azure? Is it still maintained?

@benallfree
Copy link

FYI adding

Object.defineProperty(global, 'self', {
    writable: true,
    enumerable: false,
    configurable: true,
    value: global,
});

in index.ts will also make this node-compatible!

src/fetch.ts Outdated
@@ -74,20 +74,20 @@ export function fetchEventSource(input: RequestInfo, {
let curRequestController: AbortController;
function onVisibilityChange() {
curRequestController.abort(); // close existing request on every visibility change
if (!document.hidden) {
if (!self.document?.hidden) {
Copy link

@benallfree benallfree Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add node compatibility:

// index.ts
if (typeof self === 'undefined') {
  Object.defineProperty(global, 'self', {
    writable: true,
    enumerable: false,
    configurable: true,
    value: global,
  })
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
Thanks for you comment. I had to look up the use of global. From mdn I now understand we could use globalThis, which is available everywhere, instead of self. What do you think?

NB. this PR is open since 2021-05-21, so I don't think it will be merged anytime soon.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Unfortunately I don't think this package is actively maintained, as you mentioned. I'm using a patched version of your fork right now and it works great.

Here's my patch:

diff --git a/node_modules/@microsoft/fetch-event-source/package.json b/node_modules/@microsoft/fetch-event-source/package.json
index 8528735..2e9bac3 100644
--- a/node_modules/@microsoft/fetch-event-source/package.json
+++ b/node_modules/@microsoft/fetch-event-source/package.json
@@ -9,9 +9,9 @@
   },
   "author": "Microsoft",
   "license": "MIT",
-  "main": "lib/cjs/index.js",
-  "module": "lib/esm/index.js",
-  "types": "lib/cjs/index.d.ts",
+  "main": "src/index.ts",
+  "module": "src/index.ts",
+  "types": "src/index.ts",
   "sideEffects": false,
   "scripts": {
     "clean": "rimraf ./lib ./coverage",
diff --git a/node_modules/@microsoft/fetch-event-source/src/fetch.ts b/node_modules/@microsoft/fetch-event-source/src/fetch.ts
index d6612fb..ccfdf6d 100644
--- a/node_modules/@microsoft/fetch-event-source/src/fetch.ts
+++ b/node_modules/@microsoft/fetch-event-source/src/fetch.ts
@@ -74,20 +74,20 @@ export function fetchEventSource(input: RequestInfo, {
         let curRequestController: AbortController;
         function onVisibilityChange() {
             curRequestController.abort(); // close existing request on every visibility change
-            if (!self.document?.hidden) {
+            if (!globalThis.document?.hidden) {
                 create(); // page is now visible again, recreate request.
             }
         }
 
-        if (self.document && !openWhenHidden) {
-            self.document?.addEventListener('visibilitychange', onVisibilityChange);
+        if (globalThis.document && !openWhenHidden) {
+            globalThis.document?.addEventListener('visibilitychange', onVisibilityChange);
         }
 
         let retryInterval = DefaultRetryInterval;
-        let retryTimer = 0;
+        let retryTimer : ReturnType<typeof globalThis['setTimeout']> | undefined = undefined;
         function dispose() {
-            self.document?.removeEventListener('visibilitychange', onVisibilityChange);
-            self.clearTimeout(retryTimer);
+            globalThis.document?.removeEventListener('visibilitychange', onVisibilityChange);
+            globalThis.clearTimeout(retryTimer);
             curRequestController.abort();
         }
 
@@ -97,7 +97,7 @@ export function fetchEventSource(input: RequestInfo, {
             resolve(); // don't waste time constructing/logging errors
         });
 
-        const fetch = inputFetch ?? self.fetch;
+        const fetch = inputFetch ?? globalThis.fetch;
         const onopen = inputOnOpen ?? defaultOnOpen;
         async function create() {
             if (curRequestController) {
@@ -134,8 +134,8 @@ export function fetchEventSource(input: RequestInfo, {
                     try {
                         // check if we need to retry:
                         const interval: any = onerror?.(err) ?? retryInterval;
-                        self.clearTimeout(retryTimer);
-                        retryTimer = self.setTimeout(create, interval);
+                        globalThis.clearTimeout(retryTimer);
+                        retryTimer = globalThis.setTimeout(create, interval);
                     } catch (innerErr) {
                         // we should not retry anymore:
                         dispose();

@yaxiaoliu
Copy link

Do you have any plans to support the running of service workers?

@aroman
Copy link

aroman commented Jun 27, 2023

it seems this project is abandoned, so I have forked it to make it compatible with Cloudflare Workers here and published our forked version on npm at @magiccircle/fetch-event-source-cfworker.

I do not intend to maintain this fork, but dropping a link here in case it's helpful to others!

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.

7 participants