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.

Sign up for GitHub

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

Jump to bottom

fix(cjs): include get-stream directly in bundle #770

Merged
merged 3 commits into from
Oct 19, 2023
Merged

fix(cjs): include get-stream directly in bundle #770

merged 3 commits into from
Oct 19, 2023

Conversation

kanadgupta
Copy link
Member

🧰 Changes

Seeing the following error when attempting to run a codegen'd SDK via CJS:

/Users/kanadg/Code/readmeio/tmp-api-tsup-sandbox/node_modules/@readme/api-core/dist/chunk-TOMHUGQA.cjs:138
var _getstream = require('get-stream');
                 ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/kanadg/Code/readmeio/tmp-api-tsup-sandbox/node_modules/get-stream/source/index.js from /Users/kanadg/Code/readmeio/tmp-api-tsup-sandbox/node_modules/@readme/api-core/dist/chunk-TOMHUGQA.cjs not supported.
Instead change the require of index.js in /Users/kanadg/Code/readmeio/tmp-api-tsup-sandbox/node_modules/@readme/api-core/dist/chunk-TOMHUGQA.cjs to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/Users/kanadg/Code/readmeio/tmp-api-tsup-sandbox/node_modules/@readme/api-core/dist/chunk-TOMHUGQA.cjs:138:18)
    at Object.<anonymous> (/Users/kanadg/Code/readmeio/tmp-api-tsup-sandbox/node_modules/@readme/api-core/dist/index.cjs:9:25)
    at Object.<anonymous> (/Users/kanadg/Code/readmeio/tmp-api-tsup-sandbox/.api/apis/transit-new/dist/index.js:2:16)
    at Object.<anonymous> (/Users/kanadg/Code/readmeio/tmp-api-tsup-sandbox/src/index.cjs:1:17) {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v18.18.2

This PR makes a slight tweak to our tsup config and our dependencies so the get-stream code we use is included directly in the bundle.

🧬 QA & Testing

I confirmed using our handy dandy tmp-api-tsup-sandbox that I can run CJS code once again.

@kanadgupta kanadgupta added bug Something isn't working area:core Issues related to `core`, which is the package that powers the SDKs at runtime labels Oct 19, 2023
erunion
erunion approved these changes Oct 19, 2023
View reviewed changes
packages/core/tsup.config.ts Outdated Show resolved Hide resolved
packages/core/src/lib/prepareParams.ts
@@ -9,6 +9,8 @@ import stream from 'node:stream';
import caseless from 'caseless';
import DatauriParser from 'datauri/parser.js';
import datauri from 'datauri/sync.js';
// `get-stream` is included in our bundle, see `tsup.config.ts`
// eslint-disable-next-line import/no-extraneous-dependencies
Copy link
Member

Choose a reason for hiding this comment

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

Annoying that import/no-extraneous-dependencies doesn't let us supply a list of packages to ignore.

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed... i tried playing around with our ESLint config and got nowhere

@kanadgupta @erunion
chore: pr feedback
686f1df 
Co-Authored-By: Jon Ursenbach <jon@ursenba.ch>
@erunion erunion merged commit 20727ab into main Oct 19, 2023
5 checks passed
@erunion erunion deleted the fix/get-stream branch October 19, 2023 19:09
@erunion erunion added this to the v7 milestone Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erunion erunion erunion approved these changes

Assignees
No one assigned
Labels
area:core Issues related to `core`, which is the package that powers the SDKs at runtime bug Something isn't working
Projects
None yet
Milestone
v7
Development

Successfully merging this pull request may close these issues.
2 participants
@kanadgupta @erunion