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

feat: abort controller #183

Merged
merged 4 commits into from
Nov 15, 2023
Merged

feat: abort controller #183

merged 4 commits into from
Nov 15, 2023

Conversation

kwasniew
Copy link
Contributor

About the changes

Abort controller that aborts previous request when a new one is made.

Sample use cases:

  • slow network + multiple consecutive updateContext calls
  • slow network + very short interval

Important files

Discussion points

client.updateContext({ userId: '456' }); // abort 2
await client.updateContext({ userId: '789' });

expect(abortSpy).toBeCalledTimes(2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also wanted to test error emitter on abort but the mocking fetch library does not support AbortController

@@ -89,6 +90,18 @@ export const resolveFetch = () => {
return undefined;
};

const resolveAbortController = () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

similar pattern to how we handle fetch

Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

Yes! This is very nice

return () => new globalThis.AbortController();
}
} catch (e) {
console.error('Unleash failed to resolve "AbortController" factory', e);
Copy link
Member

Choose a reason for hiding this comment

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

This should basically never happen, right? This is if someone is doing something unusual like swapping out fetch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct - someone would need to take deliberate effort to end up in this situation

@kwasniew kwasniew merged commit 4b1457c into main Nov 15, 2023
3 checks passed
@kwasniew kwasniew deleted the abort-controller branch November 15, 2023 09:53
@nammi-kobler
Copy link

I keep getting these:

image

the requests comes in pairs (within 2 seconds), and if I upgrade to v3.3.0 I get these every other request (so one is always cached and the other has this error in the console:

image

downgrading to 3.2.0 fixes these for me.

this is what Stack Overflow tells me about the NS_BINDING_ABORTED ones:
https://stackoverflow.com/questions/704561/ns-binding-aborted-shown-in-firefox-with-httpfox

@nammi-kobler
Copy link

error message in Chromium:
image

Safari:
image

@kwasniew
Copy link
Contributor Author

kwasniew commented Apr 8, 2024

@nammi-kobler those errors are expected when the requests are aborted e,g, when you're on a slow network and you have very short refreshInterval and the original request hasn't finished and you start the next one. From what I understand your refresh interval is 2s. Can you increase it and see if the problem persists? The abort operation should be an exception not a regular operation.

@nammi-kobler
Copy link

nammi-kobler commented Apr 9, 2024

The refresh interval is set to 15s, but for some reason they always come in pairs (every 15 sec), and then one of them always fails. any idea on why they would come in pairs?

@kwasniew
Copy link
Contributor Author

@nammi-kobler Can you check if this PR #207 fixes your issue? It's applied in the https://github.com/Unleash/unleash-proxy-client-js/releases/tag/v3.4.0 release

@nammi-kobler
Copy link

@nammi-kobler Can you check if this PR #207 fixes your issue? It's applied in the https://github.com/Unleash/unleash-proxy-client-js/releases/tag/v3.4.0 release

Thanks for this!
Firefox:
image

Vivaldi:
image

Safari:
image

Looks better, but it seems like it still happens over time

@kwasniew
Copy link
Contributor Author

@nammi-kobler Thanks for checking. So is it correct that the console.error is gone and now you're only left with the double request problem? Can you remind me if you use unleash-proxy-client-js directly or through react or nextjs wrapper? I couldn't reproduce this problem in this vanilla client so my guess is it's some wrapper adding those extra calls.

@nammi-kobler
Copy link

@nammi-kobler Thanks for checking. So is it correct that the console.error is gone and now you're only left with the double request problem? Can you remind me if you use unleash-proxy-client-js directly or through react or nextjs wrapper? I couldn't reproduce this problem in this vanilla client so my guess is it's some wrapper adding those extra calls.

we are using unleash-proxy-client

image

NextJS warning is from a wrapper that comes with Mantine I think?

@kwasniew
Copy link
Contributor Author

Are you running in Strict Mode by any chance? https://react.dev/reference/react/StrictMode
It runs code twice in development.

@nammi-kobler
Copy link

Are you running in Strict Mode by any chance? https://react.dev/reference/react/StrictMode It runs code twice in development.

haha yes. that's maybe the answer then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants