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!: Custom fetch support, remove node-fetch, ESM+CJS dual build, migrate to vitest, TS fixes, test improvements #162

Merged
merged 7 commits into from
Oct 8, 2024

Conversation

rolodato
Copy link
Member

@rolodato rolodato commented Oct 8, 2024

Apologies for this cursed PR - it snowballed from trying to get the fix added by #161 to typecheck.

TL;DR changes in this PR:

node-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-in fetch. This also simplifies tests by explicitly passing in a mocked fetch 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 in package.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.

@rolodato rolodato marked this pull request as draft October 8, 2024 02:59
@rolodato rolodato marked this pull request as ready for review October 8, 2024 03:09
@khvn26 khvn26 changed the title feat: Custom fetch support, remove node-fetch, ESM+CJS dual build, migrate to vitest, TS fixes, test improvements feat!: Custom fetch support, remove node-fetch, ESM+CJS dual build, migrate to vitest, TS fixes, test improvements Oct 8, 2024
@khvn26 khvn26 self-requested a review October 8, 2024 17:06
Copy link
Member

@khvn26 khvn26 left a comment

Choose a reason for hiding this comment

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

Approving this with two notions:

  • We'll be major-versioning the SDK release including this due to new Node requirement.
  • We'll perform a beta release.

@rolodato rolodato merged commit 7afadbc into main Oct 8, 2024
1 check passed
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.

2 participants