feat: migrate `api` and `core` to ESM #733

kanadgupta · 2023-09-20T19:40:33Z

🚥 Resolves RM-7989

🧰 Changes

This very much (in-progress) PR migrates the api and core subpackages over to ESM. Since we're theoretically going to be using a build tool like Rollup or tsup to generate the ESM- and CJS-flavored SDKs, we theoretically should be able to move everything here to pure ESM (but could be wrong about that, will report back 😮‍💨)

Known Issues

Knip shits the bed, seemingly when attempting to import .json files here:

api/packages/httpsnippet-client-api/test/__datasets__/application-form-encoded/index.ts

Line 4 in 61bf2de

import definition from './openapi.json';
Type discrepancy with fetch-har (hopefully should be addressed by feat: esm and cjs exports fetch-har#427):

api/packages/core/src/index.ts

Lines 125 to 126 in ec9754c

// @ts-expect-error the call signature doesn't align with the type. FIXME

return fetchHar(har as Har, {
(Obviously) failing CJS codegen tests, hopefully can get those working in feat: esm + tsup #734

🧬 QA & Testing

Provide as much information as you can on how to test what you've done.

i hate this song

erunion · 2023-10-10T00:25:10Z

packages/api/package.json

@@ -2,11 +2,13 @@
  "name": "api",
  "version": "7.0.0-alpha.3",
  "description": "Magical SDK generation from an OpenAPI definition 🪄",
+  "type": "module",


api is now ESM-only. Codegen'd SDKs will support ESM and CJS but because api is a CLI-only tool we don't need to accommodate both module flows.

erunion · 2023-10-10T00:26:25Z

packages/api/test/codegen/languages/typescript.test.ts

@@ -197,8 +196,8 @@ describe('typescript', () => {
          });
      });

-      it('should be able to make an API request (JS + CommonJS)', async () => {
-        const sdk = await import('../../__fixtures__/sdk/simple-js-cjs').then(r => r.default);
+      it.skip('should be able to make an API request (JS + CommonJS)', async () => {


These tests that are testing CJS built SDKs no longer work because of the way that these modules are handled now. These tests will get reworked in #734 when we refactor the codegen process to properly build CJS SDKs.

erunion · 2023-10-10T00:31:46Z

packages/api/test/global.d.ts

+// eslint-disable-next-line import/extensions
+import '@api/test-utils/vitest.matchers.ts';


This looks funky but because the way the test-utils package is setup if we don't have this .ts extension here then the import for these Vitest matchers don't get properly loaded. @kanadgupta and I went through a whole pairing on session on this and settled on a method of getting it working with a very complicated exports object in the package.json for that package but that ultimately broke a lot of things downstream where we're loading definitions out of @api/test-utils/definitions.

Just adding this .ts extension, and the very minor change to the Vitest config to use ../test-utils instead of @api/test-utils is a cleaner option than a flaky package config.

so weird my goodness

erunion · 2023-10-10T00:33:36Z

packages/core/package.json

+      "require": "./dist/index.cjs",
+      "import": "./dist/index.js"
+    },
+    "./errors/fetchError": {


This is the only other export because it might be nice for folks to be able to do instanceof FetchError in their implementations.

erunion · 2023-10-10T00:34:24Z

packages/core/src/index.ts

@@ -35,8 +31,6 @@ type Enumerate<N extends number, Acc extends number[] = []> = Acc['length'] exte

 export type HTTPMethodRange<F extends number, T extends number> = Exclude<Enumerate<T>, Enumerate<F>>;

-export { getJSONSchemaDefaults, parseResponse, prepareAuth, prepareParams, prepareServer };


Not only are these utilities causing issues with the CJS export of this library but also these utilities are never used outside of core internals so they don't need to be exported.

erunion · 2023-10-10T00:38:40Z

packages/core/src/index.ts

@@ -35,8 +31,6 @@ type Enumerate<N extends number, Acc extends number[] = []> = Acc['length'] exte

 export type HTTPMethodRange<F extends number, T extends number> = Exclude<Enumerate<T>, Enumerate<F>>;

-export { getJSONSchemaDefaults, parseResponse, prepareAuth, prepareParams, prepareServer };
-
 export default class APICore {


This default export alongside the named TS type exports is still causing issues with attw:

> @readme/api-core@7.0.0-alpha.3 attw > attw --pack --format table-flipped @readme/api-core v7.0.0-alpha.3 Build tools: - typescript@^5.2.2 ❓ The JavaScript appears to set both module.exports and module.exports.default for improved compatibility, but the types only reflect the latter (by using export default). This will cause TypeScript under the node16 module mode to think an extra .default property access is required, which will work at runtime but is not necessary. These types export = an object with a default property instead of using export default. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/MissingExportEquals.md 💀 Import failed to resolve to type declarations or JavaScript files. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/NoResolution.md ┌──────────────────────────────────────┬───────────────────────┬───────────────────────┬───────────────────┬───────────┐ │ │ node10 │ node16 (from CJS) │ node16 (from ESM) │ bundler │ ├──────────────────────────────────────┼───────────────────────┼───────────────────────┼───────────────────┼───────────┤ │ "@readme/api-core" │ ❓ Missing `export =` │ ❓ Missing `export =` │ 🟢 (ESM) │ 🟢 │ ├──────────────────────────────────────┼───────────────────────┼───────────────────────┼───────────────────┼───────────┤ │ "@readme/api-core/errors/fetchError" │ 💀 Resolution failed │ 🟢 (CJS) │ 🟢 (ESM) │ 🟢 │ ├──────────────────────────────────────┼───────────────────────┼───────────────────────┼───────────────────┼───────────┤ │ "@readme/api-core/package.json" │ 🟢 (JSON) │ 🟢 (JSON) │ 🟢 (JSON) │ 🟢 (JSON) │ └──────────────────────────────────────┴───────────────────────┴───────────────────────┴───────────────────┴───────────┘

Everything seems to work with the dist but it still worries me. I attempted to refactor the TS type exports out into a types.ts file, which satisfies attw but using that types file is another story:

diff --git a/packages/core/package.json b/packages/core/package.json index 5d16e2b..4e95761 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -13,6 +13,10 @@ "require": "./dist/errors/fetchError.cjs", "import": "./dist/errors/fetchError.js" }, + "./types": { + "require": "./dist/types.d.cts", + "import": "./dist/types.d.ts" + }, "./package.json": "./package.json" }, "main": "dist/index.cjs",

erunion · 2023-10-10T00:40:02Z

packages/httpsnippet-client-api/src/index.ts

@@ -74,7 +74,7 @@ function getAuthSources(operation: Operation) {
  return matchers;
 }

-export interface APIOptions {


This interface is never used outside of the library so it doesn't need to be exported.

erunion · 2023-10-10T00:41:48Z

packages/httpsnippet-client-api/src/index.ts

@@ -87,6 +87,7 @@ const client: Client<APIOptions> = {
    title: 'API',
    link: 'https://npm.im/api',
    description: 'Automatic SDK generation from an OpenAPI definition.',
+    extname: '.js',


This is a new requirement I added in @readme/httpsnippet when I upgraded it to support ESM.

erunion · 2023-10-10T00:44:12Z

packages/httpsnippet-client-api/test/index.test.ts

@@ -111,7 +110,7 @@ describe('httpsnippet-client-api', () => {
      it('should generate the expected snippet', async () => {
        const expected = await fs.readFile(path.join(DATASETS_DIR, snippet, 'output.js'), 'utf-8');

-        const code = new HTTPSnippet(mock.har).convert('node', 'api', {
+        const code = await new HTTPSnippet(mock.har).convert('node', 'api', {


Thanks to some native fetch improvements I did in @readme/httpsnippet, completely dropping support for funky FormData objects that node-fetch@2 supports, the convert function there is now async.

kanadgupta

changes look good but I don't think we need tsup for the api and core packages, more on this below!

packages/api/tsup.config.ts

kanadgupta · 2023-10-10T01:22:54Z

packages/core/tsup.config.ts

okay i also don't think we need to build this subpackage with tsup either and I think we can just rely on tsc — I was able to get pretty far in #734 with just the ESM build of this subpackage! i say this for several reasons:

to eliminate the confusing accommodations we have to make here (see this comment)

we're using tsup at the code-gen level in feat: esm + tsup #734, which will take care of bundling the ESM code and spitting out proper CJS/ESM SDKs. this pretty much eliminates the need for us to do any dual-export mess for downstream packages (with the exception of packages that we need for cjs repos like our main app, of course)

I think? that if we dont offer cjs exports of this library then cjs codegend sdks wont be able to use it because itll be esm only and tsup wont touch it because its loaded in code as a dependency

Ill change this and see how it shakes out

slept on it for a few days and i'm a bit nervous about making core ESM-only with tsc so im going to wait until the tsup codegen work is done before making any refactors to the core build process.

tsconfig.base.json

kanadgupta added 3 commits September 20, 2023 14:24

feat: migrate api and core to ESM

7d6ddb2

fix: httpsnippet-client-api and core exports

61bf2de

fix: update bin entrypoint

1f05932

kanadgupta mentioned this pull request Sep 21, 2023

chore(deps): bump #735

Merged

3 tasks

kanadgupta added 2 commits September 21, 2023 12:23

Merge branch 'main' into esm

ec9754c

fix: try rebuilding lockfile

ecc707c

i hate this song

kanadgupta mentioned this pull request Sep 21, 2023

chore(deps): bump @readme/oas-to-har from 20.1.1 to 23.0.7 #740

Closed

kanadgupta and others added 9 commits September 21, 2023 16:43

Merge branch 'main' into esm

00ef035

Merge branch 'main' into esm

2e65506

Merge branch 'main' into esm

327b20b

chore: fix lockfile

056bda9

chore: fix type

7af476b

Merge branch 'main' into esm

d25013f

Merge branch 'main' into esm

92a10a7

fix: yay remove this

b648125

Merge branch 'main' into esm

a0b2745

kanadgupta mentioned this pull request Sep 29, 2023

feat: esm + tshy #747

Closed

3 tasks

erunion added 8 commits October 6, 2023 10:33

Merge branch 'main' into esm

b2c8ff1

Merge branch 'main' into esm

af4d90d

fix: bumping our minimum httpsnippet version to v7

ce79fcd

fix: some linting issues

f4d6eac

chore(deps): bumping oas dependencies

3568fc8

feat(httpsnippet-client-api): esm compatibility

81634bf

feat(core): esm compatibility

8a63a39

fix: cleaning up how the test-utils package is built

3b8222e

kanadgupta mentioned this pull request Oct 9, 2023

feat: esm + tsup #734

Closed

7 tasks

erunion added 2 commits October 9, 2023 17:10

feat(api): esm compatibility

1a6e1bb

fix: linting issues

7ca0f70

erunion added enhancement New feature or request refactor Issues about tackling technical debt labels Oct 10, 2023

erunion added area:core Issues related to `core`, which is the package that powers the SDKs at runtime area:snippets Issues related to code snippets area:api Issues related to the `api` CLI, which builds the SDKs labels Oct 10, 2023

erunion reviewed Oct 10, 2023

View reviewed changes

fix: unskipping a test that still works fine

8c84e1b

erunion reviewed Oct 10, 2023

View reviewed changes

chore: removing unnecessary rootdir configs

c641a47

erunion reviewed Oct 10, 2023

View reviewed changes

erunion marked this pull request as ready for review October 10, 2023 00:44

erunion approved these changes Oct 10, 2023

View reviewed changes

kanadgupta commented Oct 10, 2023

View reviewed changes

erunion added 2 commits October 11, 2023 09:02

fix: building api with just tsc

8e3fd79

fix: re-enabling strict mode

b20be7b

erunion merged commit f65bc79 into main Oct 11, 2023
5 checks passed

erunion deleted the esm branch October 11, 2023 16:14

erunion added this to the v7 milestone Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrate `api` and `core` to ESM #733

feat: migrate `api` and `core` to ESM #733

kanadgupta commented Sep 20, 2023 •

edited

Loading

erunion Oct 10, 2023

erunion Oct 10, 2023 •

edited

Loading

erunion Oct 10, 2023

kanadgupta Oct 10, 2023

erunion Oct 10, 2023

erunion Oct 10, 2023

erunion Oct 10, 2023

erunion Oct 10, 2023

erunion Oct 10, 2023

erunion Oct 10, 2023

kanadgupta left a comment

kanadgupta Oct 10, 2023 •

edited

Loading

erunion Oct 10, 2023 •

edited

Loading

erunion Oct 10, 2023

erunion Oct 11, 2023

	// @ts-expect-error the call signature doesn't align with the type. FIXME
	return fetchHar(har as Har, {

		// eslint-disable-next-line import/extensions
		import '@api/test-utils/vitest.matchers.ts';

		@@ -35,8 +31,6 @@ type Enumerate<N extends number, Acc extends number[] = []> = Acc['length'] exte

		export type HTTPMethodRange<F extends number, T extends number> = Exclude<Enumerate<T>, Enumerate<F>>;

		export { getJSONSchemaDefaults, parseResponse, prepareAuth, prepareParams, prepareServer };

feat: migrate api and core to ESM #733

feat: migrate api and core to ESM #733

Conversation

kanadgupta commented Sep 20, 2023 • edited Loading

🧰 Changes

Known Issues

🧬 QA & Testing

Choose a reason for hiding this comment

erunion Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanadgupta left a comment

Choose a reason for hiding this comment

kanadgupta Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

erunion Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: migrate `api` and `core` to ESM #733

feat: migrate `api` and `core` to ESM #733

kanadgupta commented Sep 20, 2023 •

edited

Loading

erunion Oct 10, 2023 •

edited

Loading

kanadgupta Oct 10, 2023 •

edited

Loading

erunion Oct 10, 2023 •

edited

Loading