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

Build Typescript and migrate StripeResource #1539

Merged
merged 12 commits into from
Sep 1, 2022
Merged

Conversation

anniel-stripe
Copy link
Contributor

@anniel-stripe anniel-stripe commented Aug 30, 2022

Summary

r? @kamil-stripe

This PR starts the process of migrating stripe-node to Typescript through the following steps:

  1. Move files from lib/ to src/ directory
  2. Configure TypeScript compilation through tsconfig.json, outputs into lib/
  3. Add build and prepack script for Typescript compilation
  4. Add step in CI/CD to build Typescript
  5. Begin migrating existing stripe-node infrastructure to TypeScript with StripeResource, add linting overrides for .ts files

Testing

Validated that build, lint, and test scripts pass locally. Compared previous StripeResource.js to compiled lib/StripeResource.js to make sure that there are no unexpected changes.

With this change, the published NPM package will only contain the transpiled Javascript code in lib/. Our tests still import from/lib/*.js, and pass with this change.

@anniel-stripe anniel-stripe changed the title Add tsconfig, update package.json, rename lib to src, add build CI step Build Typescript Aug 30, 2022
@anniel-stripe anniel-stripe marked this pull request as ready for review August 31, 2022 18:42
@anniel-stripe anniel-stripe changed the base branch from feature/anniel-typescript to master August 31, 2022 18:42
@anniel-stripe anniel-stripe changed the title Build Typescript Build Typescript and migrate StripeResource Aug 31, 2022
package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@kamil-stripe kamil-stripe left a comment

Choose a reason for hiding this comment

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

LGTM. @dcr-stripe for quick double check?

Copy link
Contributor

@dcr-stripe dcr-stripe left a comment

Choose a reason for hiding this comment

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

LGTM overall - great work Annie! Just left some minor comments.

Let's:

  1. Set up the corresponding PR to start code-generating at src/ instead of lib/ and re-run codegen in this branch to make sure it's a no-op before merging this.
  2. Document our testing plan in this PR ie. how do we make sure the artifact we publish to NPM after this is still valid JS and compatible with existing integrations (both for JS and Typescript)

r? @anniel-stripe

tsconfig.json Show resolved Hide resolved
src/StripeResource.ts Outdated Show resolved Hide resolved
@anniel-stripe
Copy link
Contributor Author

LGTM overall - great work Annie! Just left some minor comments.

Let's:

  1. Set up the corresponding PR to start code-generating at src/ instead of lib/ and re-run codegen in this branch to make sure it's a no-op before merging this.
  2. Document our testing plan in this PR ie. how do we make sure the artifact we publish to NPM after this is still valid JS and compatible with existing integrations (both for JS and Typescript)

r? @anniel-stripe

  1. Just submitted the PR!
  2. The NPM package will only contain the transpiled Javascript code in lib/, which is what is being used in the tests (imports from /lib/*.js. All of these tests pass with this change. Will update the PR description.

Copy link
Contributor

@dcr-stripe dcr-stripe left a comment

Choose a reason for hiding this comment

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

LGTM!

@anniel-stripe anniel-stripe merged commit 7d20c8e into master Sep 1, 2022
@anniel-stripe anniel-stripe deleted the anniel-ts-build branch September 1, 2022 17:55
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.

3 participants