-
Notifications
You must be signed in to change notification settings - Fork 4
Fix types for .cjs build #16
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
Conversation
[Are the Types Wrong](https://arethetypeswrong.github.io/?p=memize%402.1.0) is currently reporting errors for memize: > Problems > > * ❌ No types: Import resolved to JavaScript files, but no type declarations were found. > > > ||"memize" > |---|---| > |node10 |✅| > |node16 (from CJS) |❌ No types| > |node16 (from ESM) |✅ (ESM)| > |bundler |✅| For more information, see Are the Types Wrong's [UntypedResolution.md](https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/UntypedResolution.md). Note that the solution is not to point both the CJS and ESM builds at the same `.d.ts` file, for reasons explained in Are the Types Wrong's [FalseESM.md](https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseESM.md). Quoting that page: > A golden rule of declaration files is that if they represent a module—that is, if they use import or export at the top level—they must represent exactly one JavaScript file. They _especially_ cannot represent JavaScript files of two different module formats. The easiest solution I found was to run `tsc` a second time to build a `.d.cts` file. For this second invocation, I pointed it at the Rollup-generated `.cjs` file. (This is because tsc and Rollup by default handle default exports differently for CJS files: tsc translates `export default foo` to a `default` attribute on an object with `__esModule: true` and requires that you instead use `export =` for a "true" CommonJS export, while Rollup [by default](https://rollupjs.org/configuration-options/#output-exports) creates a "true" default export. By pointing tsc at the Rollup-generated `.cjs` file, tsc sees Rollup's `export =`.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @joshkel ! Thanks for submitting this. At a glance, this makes sense to me. I was a bit curious about the choice to target the .cjs
built output in tsconfig.decl.cts.json
, but your explanation makes sense, and I expect it should be stable enough.
In the future, I hope to be able to go ESM-only, as support for require(esm)
goes as far back as Node.js v20, but I think this is good as an interim step that doesn't require breaking changes for Node.js support.
I'll plan to give this a more thorough review soon (no later than this weekend), and hopefully publish a fixed version shortly thereafter.
Co-authored-by: Andrew Duthie <[email protected]>
Hi @joshkel , I think these changes make sense, but I'm having trouble verifying the actual effect of these changes with a sample project linked to the changes from your branch. Would you be able to put together a sample reproducible code which demonstrates the problem of types not being loaded loaded under Here's what I tried: package.json {
"name": "memize-tmp",
"version": "1.0.0",
"type": "commonjs",
"dependencies": {
"typescript": "^5.9.2"
},
"devDependencies": {
"@types/node": "^24.1.0"
}
} tsconfig.json {
"compilerOptions": {
"module": "node16",
"moduleResolution": "node16",
"strict": true
},
"files": ["index.ts"]
} index.ts: const memize = require("memize");
memize("wrong"); Running |
Since we follow the default extensions, we shouldn't need to specify `types`.
If I use your tsconfig.json and the following index.ts: import memize from 'memize';
const memoizedFunction = memize((x: number) => {
console.log('Calculating...');
return x * 2;
});
console.log(memoizedFunction(5)); // Calculating... 10
console.log(memoizedFunction(5)); // 10 (cached result) Then the
(You can try https://github.com/joshkel/memize-types-test-case, if that helps, and assuming I didn't make any mistakes in putting it together.) |
Ah, I think the key thing was using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great! Thanks again for flagging this issue and coming up with a fix.
This is now published as |
Thanks! |
Are the Types Wrong is currently reporting errors for memize:
For more information, see Are the Types Wrong's UntypedResolution.md. Note that the solution is not to point both the CJS and ESM builds at the same
.d.ts
file, for reasons explained in Are the Types Wrong's FalseESM.md. Quoting that page:The easiest solution I found was to run
tsc
a second time to build a.d.cts
file. For this second invocation, I pointed it at the Rollup-generated.cjs
file. (This is because tsc and Rollup by default handle default exports differently for CJS files: tsc translatesexport default foo
to adefault
attribute on an object with__esModule: true
and requires that you instead useexport =
for a "true" CommonJS export, while Rollup by default creates a "true" default export. By pointing tsc at the Rollup-generated.cjs
file, tsc sees Rollup'sexport =
.)