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

Use EventTarget in worker / browser runtimes #1668

Merged
merged 19 commits into from
Feb 1, 2023
Merged

Conversation

anniel-stripe
Copy link
Contributor

@anniel-stripe anniel-stripe commented Jan 31, 2023

Summary

  1. Refactor _emitter property on the Stripe object to use EventTarget instead of EventEmitter in worker environments.
  2. Add a base class PlatformFunctions that is implemented by WebPlatformFunctions and NodePlatformFunctions
  3. Consolidate checking for streaming data and buffering logic into a tryBufferData platform function, which throws an error in workers if a ReadableStream is detected.

Workarounds because of Node <15

Event and EventTarget are only available in the global scope starting with Node 14. Since _StripeEvent needs to extend Event (to add the data object used by listener functions), I ended up creating a base class for PlatformFunctions and a separate WebPlatformFunctions that is the only place where StripeEmitter is imported.

In the tests I added to PlatformFunctions.spec.ts, I also added a check for the Node version so that WebPlatformFunctions test do not run in Node <= 14.

Testing

This gets rid of the last of the build errors when importing stripe-node in Cloudflare Workers without polyfills. I tested this new implementation in Cloudflare Workers / Pages and both emit events as expected.

@anniel-stripe anniel-stripe force-pushed the anniel-eventtarget branch 2 times, most recently from 0b9ad42 to 7507881 Compare January 31, 2023 18:25
Copy link
Contributor

@pakrym-stripe pakrym-stripe left a comment

Choose a reason for hiding this comment

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

  1. Let's refactor platform functions a bit so we don't have to push more logic into entry point.
  2. Let's add some tests to confirm parity between event emitter implementations.
  3. Let's decide what we want to do with file streams in non-node environments.

src/platform/DefaultPlatformFunctions.ts Outdated Show resolved Hide resolved
src/stripe.common.ts Outdated Show resolved Hide resolved
src/StripeEmitter.ts Show resolved Hide resolved
src/StripeEmitter.ts Outdated Show resolved Hide resolved
src/stripe.worker.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/StripeEmitter.ts Outdated Show resolved Hide resolved
src/StripeEmitter.ts Outdated Show resolved Hide resolved
src/Webhooks.ts Outdated Show resolved Hide resolved
src/stripe.common.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@pakrym-stripe pakrym-stripe left a comment

Choose a reason for hiding this comment

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

few more nits

src/StripeEmitter.ts Outdated Show resolved Hide resolved
@anniel-stripe anniel-stripe merged commit b5fc448 into master Feb 1, 2023
@remi-stripe remi-stripe deleted the anniel-eventtarget branch September 28, 2023 16:29
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