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

Include types file under conditional exports in package.json #4

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

tje
Copy link
Contributor

@tje tje commented Jul 22, 2023

I'm not super confident in what I'm doing here, but this seems to fix a warning I was running into after trying to upgrade from 0.1.0 to 1.0.0:

Could not find a declaration file for module 'hsluv'. 'node_modules/hsluv/dist/esm/hsluv.js' implicitly has an 'any' type.

There are types at 'node_modules/hsluv/dist/hsluv.d.ts', but this result could not be resolved when respecting package.json "exports". The 'hsluv' library may need to update its package.json or typings.ts(7016)

I can clearly see the types file defined already in package.json (a whole two lines away, totally not helping mitigate any shame or embarrassment in submitting this PR) - TypeScript even admits awareness of it - but for some mysterious reason it seems the existence of an adjacent exports map, uh, disqualifies it?

I'm weirdly having trouble trying to find any documentation on how this is supposed to work - the TypeScript documentation only mentions the root level types key, and Node's documentation vaguely mentions that a types file should be listed first if it's included at all under exports

I would guess that if a package could potentially define two very different exported entrypoints, maybe one shared types file is less likely to make sense? If there's a conclusive answer for this somewhere on the internet I haven't been able to fish it out from the sea of unrelated stackoverflow questions and video tutorials about how to install pytorch on a doorbell or whatever

@boronine
Copy link
Member

Hi @tje thank you for the detailed report!

I might merge your PR anyways, but I want to do my due diligence and try to reproduce the issue. I made a test repo:
https://github.com/hsluv/hsluv-js-import-test

In this repo the ESM version works (mouseover in VS Code) and the CJS version fails - shows Hsluv as type any.

(Also this seems to be relevant: microsoft/TypeScript#52363)

Are you using import or require? What version of TypeScript are you using? Did you customize "module" or "moduleResolution" in your tsconfig.json?

@tje
Copy link
Contributor Author

tje commented Jul 24, 2023

(Also this seems to be relevant: microsoft/TypeScript#52363)

Hoooow did you find that?! I promise I tried to do my research first, that looks extremely relevant

Also interesting is this linked issue: gxmari007/vite-plugin-eslint#60 - the first commit appears to reflect almost exactly what I did here, which, while it might work in some cases, is not ideal according to the comments. There's a lot to unpack here, but I'm now convinced that this PR as it is currently is not great and likely introduces a new problem


Are you using import or require? What version of TypeScript are you using? Did you customize "module" or "moduleResolution" in your tsconfig.json?

I was using import with TypeScript 5.1.6, and I did have some fancy tsconfig stuff; I was able to reproduce it in a new bare project with the following:

// package.json
{
  // ...
  "type": "module",
  "dependencies": {
    "hsluv": "^1.0.0",
    "typescript": "^5.1.6"
  }
}
// tsconfig.json
{
  "compilerOptions": {
    // These are the two settings I was using in my other project, but I've seen
    // a few other combinations ("ESNext", "NodeNext", etc.) lead to the same
    // warning
    "module": "ES2020",
    "moduleResolution": "Bundler",
    "strict": true
  }
}
// app.ts or whatever
import { Hsluv } from 'hsluv'

@boronine
Copy link
Member

Perfect, that works!

I implemented a minimal version of your repro in this branch: f75ee7f (first line passes, second line fails)

I'm going to spend a little more time researching this. Maybe steal a solution from an existing library. I did find the blog post I used for the original CJS/ESM hybrid setup:
https://www.sensedeep.com/blog/posts/2021/how-to-create-single-source-npm-module.html

I might also steal the solution from this tool:
https://antfu.me/posts/publish-esm-and-cjs#tsup
https://github.com/egoist/tsup

Wish we could drop support for CJS but would rather avoid a breaking change. If we did, the ESM module would still be loadable from CJS context using import:
https://nodejs.org/api/esm.html#import-expressions

^ If I knew that when authoring version 1.0.0 I would have avoided CJS.

@tje
Copy link
Contributor Author

tje commented Jul 26, 2023

Sorry for the late response!

I tried to write a follow-up to this yesterday but ended up getting derailed over some weirdness involving default exports:

// TypeScript correctly annotates the types for these with this PR now
import * as everything from 'hsluv'
import { Hsluv } from 'hsluv'

// ...but it also *incorrectly* types this - this package has no default export;
// attempting to import one from the ESM flavor *should* throw a warning
import hsluv from 'hsluv'

I believe what's happening here is that TypeScript is fetching the type declarations but then associating it with a CommonJS module context (since the nearest package.json is in the root, which does not explicitly specify "type": "module")

The only solution I could come up with is to duplicate the type declarations file, one for each module - it feels pretty bad to make a byte-for-byte copy of hsluv.d.ts, but I couldn't find a way to simply reuse the same one and coerce TypeScript as to which module context to treat it as. The default export problem is only a * as or {} away from not being a problem anyways

https://github.com/tje/hsluv-javascript/compare/main..export-types-per-module


I haven't seen tsup before but that looks really cool, I need to remember that one for later

I'm curious to see what you do end up with in any case! Building for cjs + esm simultaneously is almost unreasonably complicated

@boronine boronine merged commit ad48482 into hsluv:main Jul 26, 2023
@boronine
Copy link
Member

@tje I think the default export problem is due to "default" property you were providing in your package.json.

By reading the original "hybrid packaging" blog post I figured out that the reason for having separate package.json files for ESM and CJS is not applicable in our use case. So I removed that and now we have three sibling files:

  • hsluv.mjs
  • hsluv.cjs
  • hsluv.d.ts

Here's the new config:

"files": [
"dist/hsluv.d.ts",
"dist/hsluv.mjs",
"dist/hsluv.cjs",
"LICENSE",
"README.md"
],
"types": "dist/hsluv.d.ts",
"exports": {
"types": "./dist/hsluv.d.ts",
"import": "./dist/hsluv.mjs",
"require": "./dist/hsluv.cjs"
},

Here's the new test:
# Test that TypeScript can discover types with various configurations
# Discussion: https://github.com/hsluv/hsluv-javascript/pull/4
for m in "node10" "node16" "bundler"; do
(cd test/tmp && npx tsc --strict true --module es2020 --moduleResolution "$m" test.ts)
echo "tsc (--moduleResolution $m) OK"
done

And here's a release candidate: https://www.npmjs.com/package/hsluv/v/1.0.1-rc1

I guess the final verdict is that the only solution to this problem relies on undocumented behavior. We'll have to do what all the other libraries do and just roll with it!

Can you let me know if the v1.0.1-rc1 works for you?

@tje
Copy link
Contributor Author

tje commented Jul 27, 2023

Just tried it out and it works! The accursed red squiggly line is gone!

@boronine
Copy link
Member

Okay it's out: https://www.npmjs.com/package/hsluv/v/1.0.1

Thank you for your help with this release!

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