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

Manual type declarations in openapi-fetch #1424

Merged
merged 3 commits into from
Nov 3, 2023
Merged

Manual type declarations in openapi-fetch #1424

merged 3 commits into from
Nov 3, 2023

Conversation

drwpow
Copy link
Contributor

@drwpow drwpow commented Nov 3, 2023

Changes

Splits index.ts (runtime + types) into manual index.js and index.d.ts files. Added this section to the CONTRIBUTING docs to explain why:

This project uses manual type declarations (docs) that are kept separately from the runtime code. index.d.ts contains type declarations, and index.js contains runtime code. The most important code lives in index.d.ts because this library exists to provide correct type inference. index.js, is far less important, and is only doing bare-minimum work to support the API as efficiently as possible.

In most projects, this is not recommended practice as the two can (and will) diverge, and you’re left with types that “lie” to you about what the runtime is doing. So this project does have that liability. However, due to the unique nature of this project implementing complex type inference from user-provided schemas, the type inference itself is so complex it was interfering with readable, maintainable, runtime code. By separating the two we won’t have the more complex parts of TypeScript interfering with the optimal runtime code. This would not be tenable in a larger project, but is perfect for a small codebase like this with minimal surface area.

When writing code, it’s tempting to ignore index.d.ts and only make runtime updates, since that is generally simpler. But please don’t! We write tests in *.test.ts files intentionally so that the type inference can be typechecked as well as the runtime. Whenever you make a change to index.js, you probably need to also make a chance to index.d.ts, too. And testing type correctness in tests is just as important as testing the runtime!

This is an internal change—tests still pass—but it will make some upcoming changes much, much easier.

How to Review

  • Tests should pass

Checklist

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

Copy link

changeset-bot bot commented Nov 3, 2023

🦋 Changeset detected

Latest commit: 3347010

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

This PR includes changesets to release 1 package
Name Type
openapi-fetch 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

@@ -3,7 +3,7 @@ import { afterEach, beforeAll, describe, expect, it, vi } from "vitest";
// @ts-expect-error
import createFetchMock from "vitest-fetch-mock";
import createClient from "../src/index.js";
import type { paths } from "./v1.d.js";
import type { paths } from "./fixtures/api.js";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved into /fixtures/ folder for cleanup

@@ -7,6 +7,7 @@
"lib": ["ESNext", "DOM"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fun fact: I originally wanted to add allowJs: true to TSConfig, however, that broke everything—it tries to infer types from .js files and ignores .d.ts. So fortunately no change is necessary, and TypeScript likes .d.ts just as much as the original .ts


This will ask if it’s a `patch`, `minor`, or `major` change ([semver](https://semver.org/)), along with a plain description of what you did. Commit this new file along with the rest of your PR, and during the next release this will go into the official changelog!

## Opening a Pull Request
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not actually rewritten; just moved several sections out of the “Opening a Pull Request” heading and into a “Writing Code” heading

Copy link

cloudflare-workers-and-pages bot commented Nov 3, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3347010
Status: ✅  Deploy successful!
Preview URL: https://cce5cc96.openapi-ts.pages.dev
Branch Preview URL: https://manual-types.openapi-ts.pages.dev

View logs

@drwpow drwpow merged commit 8f5adb3 into main Nov 3, 2023
8 checks passed
@drwpow drwpow deleted the manual-types branch November 3, 2023 15:25
@github-actions github-actions bot mentioned this pull request Nov 3, 2023
@drwpow drwpow mentioned this pull request Nov 3, 2023
1 task
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.

1 participant