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

Introduce separate entry point for worker environments #1660

Merged
merged 21 commits into from
Jan 19, 2023

Conversation

anniel-stripe
Copy link
Contributor

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

Summary

Introduces an exports field to the package.json that defines a separate entry point for environments where Node.js built-in modules are not supported.

Major changes in this PR:

  • Define two entrypoint files: stripe.ts and stripe.worker.ts, move common code into stripe.common.ts
  • Separate functions defined in utils.ts that depend on Node.js built-ins to the platform directory in a NodePlatformFunctions class that inherits from DefaultPlatformFunctions
  • Add private _platformFunctions property onto Stripe object. Use this to override functions that depend on Node-specific modules in stripe.node.ts
  • Initialize Stripe.createNodeHttpClient and Stripe.createNodeCryptoProvider to throw errors and override them in stripe.node.ts (won't be available for workers)
  • Create generic Stripe.createHttpClient that defaults to Stripe.createNodeHttpClient normally, and Stripe.createFetchHttpClient in workers
  • Convert affected tests to TypeScript

Testing

I wasn't sure of a good way to add an automated test that mimics a worker environment. I tested this locally, and validated that the expected build errors have been resolved (see next section 'Remaining Work' for remaining errors).

The original build errors:
Screen Shot 2023-01-17 at 12 46 53 PM

Now:
Screen Shot 2023-01-19 at 10 52 44 AM

Remaining Work

  • Refactor Stripe._emitter and utils.checkForStream to not use EventEmitter for worker environments

src/stripe.common.ts Outdated Show resolved Hide resolved
src/stripe.common.ts Outdated Show resolved Hide resolved
src/stripe.common.ts Outdated Show resolved Hide resolved
src/stripe.default.ts Outdated Show resolved Hide resolved
src/nodeUtils.ts Outdated Show resolved Hide resolved
@pakrym-stripe
Copy link
Contributor

pakrym-stripe commented Jan 17, 2023

Why is node 12 on fire? Is it because it's getting the wrong HTTP client?

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.

Mostly code structure related comments.

@pakrym-stripe
Copy link
Contributor

I wasn't sure of a good way to add an automated test that mimics a worker environment.

Create a test project similar to ones we have and invoke the cloudflare build CLI on it from tests.

src/nodeUtils.ts Outdated Show resolved Hide resolved
src/nodeUtils.ts Outdated Show resolved Hide resolved
src/Webhooks.ts Outdated Show resolved Hide resolved
src/stripe.node.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 comments. This is getting better and better with every commit!

src/Webhooks.ts Outdated Show resolved Hide resolved
src/stripe.common.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
@anniel-stripe anniel-stripe force-pushed the anniel-conditional-exports branch from b3a8791 to 94369db Compare January 19, 2023 08:24
src/Webhooks.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.

Great work! 🎉

@anniel-stripe anniel-stripe enabled auto-merge (squash) January 19, 2023 22:59
@anniel-stripe anniel-stripe merged commit 5f2eec2 into master Jan 19, 2023
@anniel-stripe anniel-stripe deleted the anniel-conditional-exports branch January 19, 2023 23:23
gurus00 pushed a commit to gurus00/stripe-node that referenced this pull request Feb 11, 2025
* Separate stripe into worker and default entrypoints, needs tests for worker

* Fix test, remove unused import

* Actually require child_process

* build

* Convert tests to TypeScript, add browser field

* Move more utils functions to nodeUtils

* Use _utils defined on Stripe object

* Add error messages to createNodeHttpClient and createNodeCryptoProvider

* Move platform-specific utils to new PlatformFunctions classes, add tests

* Abstract getUname to platform functions, set _platformFunctions on Webhooks

* Make generic createCryptoProvider in Webhooks

* Revert "Make generic createCryptoProvider in Webhooks"

This reverts commit c8af4d5.

* Remove stale file in lib

* Create generic createHttpClient that creates FetchHttpClient by default in workers

* Move getUname to PlatformFunctions (now returns a promise), and misc reviewer comments

* Make generic createCryptoProvider in Webhooks

* Removing this alias

* Fix DefaultPlatformFunctions getUname, fix test

* Add getMockPlatformFunctions test util, prefix _createCryptoProvider
gurus00 pushed a commit to gurus00/stripe-node that referenced this pull request Feb 11, 2025
* Separate stripe into worker and default entrypoints, needs tests for worker

* Fix test, remove unused import

* Actually require child_process

* build

* Convert tests to TypeScript, add browser field

* Move more utils functions to nodeUtils

* Use _utils defined on Stripe object

* Add error messages to createNodeHttpClient and createNodeCryptoProvider

* Move platform-specific utils to new PlatformFunctions classes, add tests

* Abstract getUname to platform functions, set _platformFunctions on Webhooks

* Make generic createCryptoProvider in Webhooks

* Revert "Make generic createCryptoProvider in Webhooks"

This reverts commit c8af4d5.

* Remove stale file in lib

* Create generic createHttpClient that creates FetchHttpClient by default in workers

* Move getUname to PlatformFunctions (now returns a promise), and misc reviewer comments

* Make generic createCryptoProvider in Webhooks

* Removing this alias

* Fix DefaultPlatformFunctions getUname, fix test

* Add getMockPlatformFunctions test util, prefix _createCryptoProvider
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