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

Separate outputs for CommonJS and ESM when building openapi-typescrip… #1479

Merged
merged 4 commits into from
Dec 13, 2023

Conversation

darwish
Copy link
Contributor

@darwish darwish commented Dec 4, 2023

…t-helpers

Changes

Fixes #1478

Creates separate output directories for CommonJS and ESM for the output of openapi-typescript-helpers.

How to Review

  1. Run pnpm build
  2. Check that both packages/openapi-typescript-helpers/dist/ and packages/openapi-typescript-helpers/dist/cjs/ contain an index.ts and an index.d.ts file.

Checklist

  • Unit tests updated
  • docs/ updated (if necessary)
  • pnpm run update:examples run (only applicable for openapi-typescript)

Copy link

changeset-bot bot commented Dec 4, 2023

🦋 Changeset detected

Latest commit: 935d4e7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
openapi-fetch Patch
openapi-typescript-helpers Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Owner

@drwpow drwpow left a comment

Choose a reason for hiding this comment

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

Nice fix! Thank you!

@drwpow
Copy link
Owner

drwpow commented Dec 6, 2023

Hm the failing tests might be from a race condition while building openapi-fetch and openapi-typescript-helpers. It seems that because openapi-typescript-helpers now requires a build step to work that it didn’t need before, it has to happen before openapi-fetch.

As a workaround, in the root, I’d recommend doing something like:

  "scripts": {
    "build": "run-p -s build:*",
    "build:openapi-typescript": "cd packages/openapi-typescript && pnpm run build",
-   "build:openapi-typescript-helpers": "cd packages/openapi-typescript-helpers && pnpm run build",
-   "build:openapi-fetch": "cd packages/openapi-fetch && pnpm run build",
+   "build:openapi-fetch": "cd packages/openapi-typescript-helpers && pnpm run build && cd ../openapi-fetch && pnpm run build"

The TS helpers build is basically instant/trivial. And this keeps openapi-fetch and openapi-typescript built in parallel, which while both are quick, still makes a noticeable difference in CI runs

@darwish
Copy link
Contributor Author

darwish commented Dec 6, 2023

Thanks, I've applied your fix.

@drwpow
Copy link
Owner

drwpow commented Dec 6, 2023

Hm something else seems to be going on. Will take a closer look later

Copy link
Owner

@drwpow drwpow left a comment

Choose a reason for hiding this comment

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

This is working for me locally, and seems to just be a weird CI issue. I’ll merge this, then poke at it separately.

@drwpow drwpow merged commit c6d945b into drwpow:main Dec 13, 2023
2 of 7 checks passed
@drwpow
Copy link
Owner

drwpow commented Dec 13, 2023

Oh I figured it out—it was a CI thing. It was just in the test runs, each individual package was building itself but not others (i.e. we updated it in the wrong place, the building of openapi-typescript-helpers needed to happen inside openapi-fetch’s package.json itself).

But really it’s just a flimsy setup; I’ll make some changes that make that better (also we shouldn’t have a prepare step on openapi-fetch itself)

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.

Error when compiling for CJS: "The current file is a CommonJS module..."
2 participants