build(openapi-fetch): bundle commonjs

[kecrily]

Changes

What does this PR change? Link to any related issue(s).

Also to fix #1102, but unlike #1103, this PR reuses esbuild instead of adding a new dependency.

Just change the --format flag to bundle cjs.

Ref https://esbuild.github.io/api/#format-commonjs

How to Review

How can a reviewer review your changes? What should be kept in mind for this review?

Is this type of bundling acceptable?

Checklist

N/A

• Unit tests updated
• README updated
• examples/ directory updated (only applicable for openapi-typescript)

Other

If your project/package is being blocked by the lack of cjs distribution for openapi-fetch, you can temporarily try using @kecrily/openapi-fetch based on this PR before this PR is merged.

[changeset-bot]

⚠️ No Changeset found

Latest commit: fd27005

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[drwpow]

I’m on board for this addition, so long as we have the CLI tests testing this CommonJS bundle as well.

The major barrier for this existing in v6 wasn’t the actual bundling of it; it was maintenance, and getting bug reports for CommonJS that I don’t have time to support. Any way we can mitigate that by having the test suite ensure it works would help (FWIW, I don’t think the unit tests need to be duplicated; just the CLI tests expanded to test this bundle).

[kecrily]

so long as we have the CLI tests testing this CommonJS bundle as well.

What does the CLI test refer to here? It means to test if esbuild can bundle cjs properly?

The major barrier for this existing in v6 wasn’t the actual bundling of it; it was maintenance, and getting bug reports for CommonJS that I don’t have time to support.

We use cjs in our projects, and if we find a problem, we will try to fix it and submit a PR if possible.

[drwpow]

Oh the CLI test here: packages/openapi-typescript/test/cli.test.ts

Should be as simple as duplicating those tests and setting cmd to the CJS build (but obviously keep the existing build tests in place).

We use cjs in our projects, and if we find a problem, we will try to fix it and submit a PR if possible.

That would be enormously helpful, thank you 🙂

[kecrily]

Should be as simple as duplicating those tests and setting cmd to the CJS build (but obviously keep the existing build tests in place).

This PR is not about openapi-typescript. this cjs test seems to be non-existent. Also I don't think openapi-typescript should distribute a cjs version, since it is usually only used on the command line. (Of course if you think this is necessary, I'd be happy to modify it together)

[drwpow]

🤦 Ah I’m so sorry—I’m still getting used to these projects being merged together and I was hopping back-and-forth between package conversations today. This looks great! Sorry for my confusion.

[drwpow]

Also I’m fine with adding this, but this is more of an “invalid” / “legacy” property which was never officially standardized, which was why it was missing in the first place. It’s also not needed because it’s redundant with "main", but it’s also fine to include just in case it helps something.

[drwpow]

Released in v0.4.0 :tada. Thank you!

[dino-rodriguez]

@drwpow How do I actually get the cjs output to be created? When I install the module using yarn and use it in a script, it can't find the .cjs files because they don't seem to exist when I try to execute the script:

Error: Cannot find module '/Users/dino/Code/indie/api/client/node_modules/openapi-fetch/dist/index.cjs'
    at createEsmNotFoundErr (node:internal/modules/cjs/loader:1016:15)
    at finalizeEsmResolution (node:internal/modules/cjs/loader:1009:15)
    at resolveExports (node:internal/modules/cjs/loader:529:14)
    at Function.Module._findPath (node:internal/modules/cjs/loader:569:31)
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:981:27)
    at Function.Module._resolveFilename.sharedData.moduleResolveFilenameHook.installedValue [as _resolveFilename] (/Users/dino/.volta/tools/image/packages/ts-node/lib/node_modules/@cspotcode/source-map-support/source-map-support.js:811:30)
    at Function.Module._load (node:internal/modules/cjs/loader:841:27)
    at Module.require (node:internal/modules/cjs/loader:1061:19)
    at require (node:internal/modules/cjs/helpers:103:18)
    at Object.<anonymous> (/Users/dino/Code/indie/api/client/client.ts:1:1) {
  code: 'MODULE_NOT_FOUND',
  path: '/Users/dino/Code/indie/api/client/node_modules/openapi-fetch/package.json'

Confirmed on the latest version.

[drwpow]

Ah sorry—this is a problem with the automated releases; not a problem with this PR at all. Will fix.

[drwpow]

Thanks for bearing with me on the automated releases. It’s a PITA to set up, and there are usually bugs like this for the first month or so. But long-term it’s much more sustainable. And improves the quality of the library because maintainers can release as well.

[dino-rodriguez]

@drwpow No worries at all - thank you for responding so quickly! Hugely appreciate it