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

Dependency "cockatiel" does not work with Cloudflare Workers #3

Open
davidhouweling opened this issue Nov 3, 2024 · 4 comments
Open

Comments

@davidhouweling
Copy link
Contributor

Context

I'm working on creating an app using Cloudflare Wworkers and there is a collision with AbortController within cocatiel.

I could try and raise it with the owners there but i thought it might be quicker to start here.

I understand that it is presently used as a retry mechanism, I wanted to understand what the need for it was and whether you would be okay with dropping it, or perhaps doing a rudimentary retry without the use of cocatiel?

I do have a working approach locally where I've removed cocatiel but it no longer has the retry mechanism.

Thoughts on having me raise a PR to drop it?

@GravlLift
Copy link
Owner

I don't think I want to remove retry functionality entirely, but I could probably be convinced to make it configurable so that the package can be used without it.

Maybe you can link me to a reproduction example of what you're facing? It doesn't have to be your full project, a minimal example would work too.

@davidhouweling
Copy link
Contributor Author

davidhouweling commented Dec 30, 2024

Sorry been a while, coming back to this now... I've set up this example repo: https://github.com/davidhouweling/cloudflare-worker-halo-infinite-api

In the main branch is the issue as outlined (though i'm not sure if more needs to be done to get Cloudflare Workers set up locally given that I've done it in the past so you may need to follow the getting started guide more closely... though you don't need to deploy to replicate the issue).

I also included a monkey-patch branch which uses patch-package to override the current setup with the native fetch client.

Money patch original code change can be seen in https://github.com/davidhouweling/halo-infinite-api/tree/remove-request-policy

@GravlLift
Copy link
Owner

If you wanted to simply override the native fetch, there is an optional parameter on all of the clients for doing just that. For instance, the HaloAuthenticationClient.

private readonly clearToken: () => Promise<void>,
private readonly fetchFn: FetchFunction = defaultFetch
) {}

But it seems like native fetch is not actually your problem. Rather, the fact that cockatiel calls new AbortController() on import doesn't seem like it jives with Cloudflare workers.

In between when you opened this issue and now it looks like you are not the only person who has noticed this, and someone else opened an issue with Cloudflare on this exact issue. I don't know much about the Cloudflare worker, so I can't say for certain, but that has all the smells of a bug in cloudflare rather than anything cockatiel is specifically doing wrong.

As far as my idea to make cockatiel optional, that won't save you either. Since Cloudflare is bundling all code, even a conditional import will still evaluate that new AbortController() code on startup, so the only approach that would help you is to completely remove cockatiel from the codebase, similar to what you have done in your patch branch. I don't love that the library loses auto-retrying of expired tokens as a consequence (and presumably you're needing to handle those 401s in your own code somewhere now as a result).

The only thing I can think to do is to publish my cockatiel-based retry policy as a completely separate package, then leave it up to the programmer to import it and pass a specific retry policy to the client. That way one could write code that completely skips over import 'cockatiel', and thus the new AbortController() never runs.

I think given that the narrow impact of this issue (Cloudflare workers), and the strong possibility that CF will patch this issue entirely, I'm going to hold off on making any such change at the moment. I'll leave this issue open until we get a response from them though.

@davidhouweling
Copy link
Contributor Author

Yep I came to the same conclusion as you, which is why I went with a patch package approach. Similar to your summary, I don't think it's necessarily your responsibility to fix and the intersection between CF and this is quite small (I'm likely the only one at the moment) so at least it's documented here if someone needs the work around.

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

No branches or pull requests

2 participants