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

chore: Simplify type generation #23

Merged
merged 3 commits into from
May 22, 2024
Merged

chore: Simplify type generation #23

merged 3 commits into from
May 22, 2024

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented May 14, 2024

This simplifies the generation of type definitions.

  1. Uses a // @ts-self-types comment to specify types in the rolled-up packages, eliminating the need for a separate step to add a triple-slash directive.
  2. Eliminates all tsconfig.cjs.json configurations. It turns out we can just copy over index.d.ts to index.d.cts and TypeScript will be happy.

@harish-sethuraman
Copy link
Member

  1. Sorry to ask again but I'm kind of confused with so many of these ts configs and stuff. I can still see 3 files index.d.ts type.d.ts and a type.ts file.
  • Index.js refers types to index.d.ts
  • index.d.ts refers to types inside type.ts

in that case why do we have type.d.ts (this seems like exact replica of type.ts)

  1. One more thing that I noticed is I removed all tsconfig.json and built the package just with plain rollup and still can see type.ts in the esm and cjs of dist folder as index.js imports declarations. In that case should we actually remove the copy plugin that does the exact same thing?

@nzakas
Copy link
Member Author

nzakas commented May 15, 2024

in that case why do we have type.d.ts (this seems like exact replica of type.ts)

tsc generates this. Maybe it's not needed, but I don't see the harm in leaving it. No need to add another step to the process.

2. One more thing that I noticed is I removed all tsconfig.json and built the package just with plain rollup and still can see type.ts in the esm and cjs of dist folder as index.js imports declarations. In that case should we actually remove the copy plugin that does the exact same thing?

I'm guessing that you didn't delete dist before doing this. If you delete dist and try npm run build without Rollup copying over types.ts, you'll get a tsc error.

@@ -9,7 +9,8 @@
"lint": "eslint .",
"lint:fix": "eslint --fix .",
"fmt": "prettier --write .",
"fmt:check": "prettier --check ."
"fmt:check": "prettier --check .",
"test:jsr": "npm run test:jsr --workspaces --if-present"
Copy link
Member

Choose a reason for hiding this comment

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

consider adding test:jsr to the ci?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea 👍

packages/object-schema/package.json Show resolved Hide resolved
Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM. Waiting a few days before merging.

@aladdin-add aladdin-add merged commit 5f0d2e2 into main May 22, 2024
14 checks passed
@aladdin-add aladdin-add deleted the types-simplification branch May 22, 2024 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants