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

fix: add types #248

Merged
merged 1 commit into from
Jan 14, 2023
Merged

fix: add types #248

merged 1 commit into from
Jan 14, 2023

Conversation

unional
Copy link
Contributor

@unional unional commented Jan 14, 2023

Add types to the package.

TypeScript need this to read the type definition.

btw, the ESM one likely using module: Node16 should be the correct value, instead of ESNext.

But that can be a different PR.

@isaacs
Copy link
Owner

isaacs commented Jan 14, 2023

Huh, I have no reason to object to this, but I'm curious why it doesn't pull in types for you just by virtue of them being in the same folder as the module being loaded. I see them getting pulled in with typescript 4.9.3, what version are you using?

@isaacs
Copy link
Owner

isaacs commented Jan 14, 2023

btw, the ESM one likely using module: Node16 should be the correct value, instead of ESNext.

The ESM build usings tsconfig-esm.json, which has:

{
  "extends": "./tsconfig-base.json",
  "compilerOptions": {
    "module": "esnext",
    "outDir": "dist/mjs"
  }
}

So yeah, it's using esnext. If it was using node16, it wouldn't produce esm modules.

PR-URL: isaacs#248
Credit: @unional
Close: isaacs#248
Reviewed-by: @isaacs
@isaacs isaacs closed this in 8a3a481 Jan 14, 2023
@isaacs isaacs merged commit 8a3a481 into isaacs:main Jan 14, 2023
@unional unional deleted the types branch January 14, 2023 21:26
@unional
Copy link
Contributor Author

unional commented Jan 14, 2023

Um, interesting. It is possible that it is improved in some newer versions of TypeScript. It was needed for many versions and I help a lot of packages to add the type definitions, including my own.

Also for ESM, when I start converting my packages to dual modules a few months ago, I definitely notice they are needed, especially for subpath exports.

e.g. the following didn't work when consuming as ESM when I tried it:

{
  "exports": {
    "." : { /* no "types" field */ }
  },
  "main": "...",
  "types": "..."
}

If it works without those fields, feel free to remove them. Less is more. 🍻

@isaacs
Copy link
Owner

isaacs commented Jan 14, 2023

It is possible that it is improved in some newer versions of TypeScript. It was needed for many versions and I help a lot of packages to add the type definitions, including my own.

That's my guess. No real harm in having it there, if it makes some older versions of TypeScript work properly, then that's good enough reason to keep it.

I am noticing that the same exact .d.ts files get duplicated using my hybrid setup, I wonder if there's a way to have one of them reference the other. Tricky when there's more than one module exported, though.

@unional
Copy link
Contributor Author

unional commented Jan 14, 2023

I am noticing that the same exact .d.ts files get duplicated using my hybrid setup, I wonder if there's a way to have one of them reference the other. Tricky when there's more than one module exported, though.

There are two ways to do it:

  1. set declaration to false in tsconfig for CJS
  2. Use package.json files glob

Nowadays I use the second approach in my packages because I want to make sure everything are compiled correctly, including tests.

Adding !/dist/cjs/**/*.d.ts there will fix the issue.

UPDATE: actually, don't do that. I'm working on a demo that you may need both:
https://github.com/cyberuni/typescript-module-resolutions-demo

@unional
Copy link
Contributor Author

unional commented Jan 14, 2023

@isaacs 👆 pls ignore what I said about removing one set of declaration.

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.

2 participants