feat!: Custom fetch support, remove node-fetch, ESM+CJS dual build, migrate to vitest, TS fixes, test improvements #162
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Apologies for this cursed PR - it snowballed from trying to get the fix added by #161 to typecheck.
TL;DR changes in this PR:
fetch
implementation@ts-ignore
snode-http only supports
signal
in v3, which only supports ESM. Instead of bothering with node-http breaking changes and incompatibility between node-http and built-in fetch types, I removed it altogether in favour of built-infetch
. This also simplifies tests by explicitly passing in a mockedfetch
implementation and not needing to mock globals, which doesn't play nice with TS.By depending on built-in fetch and migrating to ESM, we're now requiring Node >= 18, which is enforced in
engines
inpackage.json
. The bulk of the changes to imports are for making file extensions explicit, which are mandated by ESM.Building CJS in addition to ESM lets us continue supporting use of
require
, which we're still using in our docs. The dual-build ESM+CJS approach is the same that https://github.com/auth0/node-auth0 is using, which I used as a reference.There are no user-facing API breaking changes in this PR, but the removal of node-fetch and requirement of Node >= 18 might warrant a major version bump. I would like to release this as a v4 beta and get some real-world testing.
There's 1 minor bugfix in
sdk/polling_manager.ts
. We were fetching the environment document twice immediately on startup. This extra call has been removed.We're also now down to just 1
@ts-ignore
in the whole project.