Add cloudflare worker compatibility

[jesseditson]

See/Fixes #215

In existing code, this lib detects the fetch API by checking for window - however, there are a few more environments where fetch is now the native http handlingmethod and this library could work automatically. Namely cloudflare workers (v8 isolates) and Web/Service Workers.

This PR changes the feature detection to also include WorkerGlobalScope environments, which will detect the above cases.

In addition to the detection change, cloudflare workers appear to hoist require calls, so this PR also manually hoists the require calls that were previously gated behind an early return in the browser case, but wraps them in a try/catch so that if we incorrectly feature detect node, it will preserve the original error and re-throw it once we've passed the feature gate.

[jesseditson]

Hey @ggalmazor! Curious if you're currently maintainer and if you could take a look! I've self-published this for now so will move on to other things, but I suspect this will help others.

[ggalmazor]

Hi @jesseditson,

Thanks for working on this! We really appreciate it :)

I will review it and get back to you as soon as possible.

[ggalmazor]

Hi @jesseditson! I just noticed that the CI build didn't run for this PR yet. Our security policies require manual approval to run workflows for PRs from public repos like yours. Apologies for that!

While reviewing your changes on my computer, I noticed the tests are failing. It looks like nock is not providing stubbed responses for API requests in our tests, which are reaching DNSimple's live environment and failing.

Did this happen in your setup, too? I'm afraid we can't merge the PR with failing tests otherwise.

After considering your PR, we're also evaluating the option of simplifying this library by removing the feature detection altogether and letting end-users provide a Fetcher implementation if their runtime doesn't have fetch. What do you think about it?

[jesseditson]

Nice to have CI here! I thought I ran the tests locally to make sure I didn't regress, but lemme see if I can expand the suite locally to see what's going on with the CI failure.

An injected fetch API would also solve this for me so I'd be happy to move to that API instead if that route is preferable on your end!

[ggalmazor]

An injected fetch API would also solve this for me, so I'd be happy to move to that API instead if that route is preferable on your end!

We also see this as a desirable simplification, but we need to work to introduce the change in the safest way possible. In the meantime, your PR would be a valuable addition. We would happily ship this PR if you can fix the issues anyway.

[ggalmazor]

Hey, @jesseditson, after working for a bit on the code to inject a fetch, I realized that we have an issue with nock not fully supporting fetch (at least the way it is defined right now in this project).

I'm migrating the tests to fetch-mock and introducing a default Fetcher implementation based on whatever built-in fetch we can find at runtime.

[jesseditson]

Sounds good! Should I take a look at getting this PR to pass CI still or hold off until the nock changes land?

[ggalmazor]

Yes, we can punt on that for now. I just created #217 with the migration to fetch-mock and once that's merged, you can try rebasing this PR, which may fix the spec failures altogether.

[ggalmazor]

@jesseditson , I just created #218 using your suggestions from this PR. Let me know what you think :)

Ultimately, we decided to leave the https based implementation as a fallback if fetch is not available to prevent introducing breaking changes to users who might rely on our current default behavior.

On the other hand, end-users can always inject a Fetcher through the DNSimple class constructor.

[jesseditson]

Superceded by #216 !