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(sdk-snippets): library modernization with ESM+CJS builds #1017

Closed
wants to merge 11 commits into from

Conversation

erunion
Copy link
Member

@erunion erunion commented Jun 16, 2024

🧰 Changes

This modernizes our sdk-snippets library in order to now export CJS and ESM dist builds.

🧬 QA & Testing

Running npm run attw all of the types for this new library output line up as expected:

Screen Shot 2024-06-16 at 12 19 51 AM

@erunion erunion added the snippets Issues related to our SDK snippet generation label Jun 16, 2024
@erunion erunion added the enhancement New feature or request label Jun 16, 2024
Copy link
Member

@domharrington domharrington left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

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

a few small suggestions but LGTM for the most part!

packages/sdk-snippets/tsup.config.ts Outdated Show resolved Hide resolved
packages/sdk-snippets/tsup.config.ts Outdated Show resolved Hide resolved
packages/sdk-snippets/src/index.ts Show resolved Hide resolved
Copy link
Member

@gratcliff gratcliff left a comment

Choose a reason for hiding this comment

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

💙

@erunion erunion requested a review from gratcliff July 26, 2024 22:56
@erunion erunion requested a review from kanadgupta July 28, 2024 07:48
@kanadgupta
Copy link
Member

Given that this package is really only used in our main codebase, is this PR necessary? Historically we've really only done this dual-export thing for packages that are used by @readme/api-core

@erunion
Copy link
Member Author

erunion commented Jul 29, 2024

Not really I guess. I'll close it out

@erunion
Copy link
Member Author

erunion commented Jul 29, 2024

@kanadgupta I'm remembering now that the reason I did this was to upgrade to the latest release of httpsnippet which has these dual dists. I guess I can refactor this to retain CJS dists but upgrade our TS module resolution to node16 in order to load httpsnippet again.

@kanadgupta
Copy link
Member

I guess I can refactor this to retain CJS dists but upgrade our TS module resolution to node16 in order to load httpsnippet again.

yep, agreed with this!

@erunion erunion closed this Jul 29, 2024
@erunion erunion deleted the feat/modernize-sdk-snippets branch July 30, 2024 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request snippets Issues related to our SDK snippet generation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants