fix: more typedefs improvements by plbstl · Pull Request #366 · pillarjs/iconv-lite

plbstl · 2025-12-18T04:29:09Z

Like in #330 (comment), the changes made in https://github.com/plbstl/iconv-lite/tree/more-typedefs-improvements resolves the issue.

Changing only the typings will not fully resolve the issue, as the JavaScript code will behave differently at runtime. The tests still pass, and were (technically) not modified.

You can see in https://github.com/nodejs/cjs-module-lexer's README, that module.exports, module.exports.x and exports.x are shown. That's the right way to export CommonJS modules.

cc @bjohansebas @prigaux @takayukioda @kyota-fujikura @Nirator78 . Can you review and verify that these changes work in the different environments and configs?

Closes #363

plbstl · 2025-12-18T04:32:40Z

Just saw #363, #366 and #364 .

kyota-fujikura · 2025-12-18T10:01:59Z

Thanks!

I tested commit 519fd3b locally using this reproduction repo:
https://github.com/kyota-fujikura/iconv-lite-type-error-reproduction

Here’s the matrix I tried:

package.json: "type": "module" / "type": "commonjs"
tsconfig.json:
- "module": "nodenext" (I also tried "es2022" and "commonjs"; the behavior aligned with the corresponding type setting)
- "esModuleInterop": true / false
Import style: default import / namespace import

With this PR applied, both tsc and runtime execution succeeded for almost all combinations I tried. I did find one mismatch, though:

default import with commonjs + "esModuleInterop": false
-> tsc succeeds, but Node fails at runtime.

(I'm not sure how common this configuration is in practice.)

In my local testing, commenting out the export { iconv as default } in index.d.ts made the behavior consistent for that configuration (both tsc and runtime fail). Also, even without that line, default import still worked in the other configurations I tested.

https://www.typescriptlang.org/tsconfig/#esModuleInterop

…f `iconv` in `test/types/iconv.ts`

plbstl · 2025-12-18T12:00:33Z

@kyota-fujikura Thank you very much for the feedback.

And you're right. Removing export { iconv as default } ensures the correct behavior (both tsc and runtime fail). I just pushed the relevant changes.

After cloning https://github.com/kyota-fujikura/iconv-lite-type-error-reproduction, I also added a src/named-import.ts file:

import { encode, decode } from "iconv-lite";

const input = "こんにちは";
const encoding = "shift_jis";

const encoded = encode(input, encoding);
const decoded = decode(encoded, encoding);

console.log({ encoded, decoded });

and scritps for running the files directly with NodeJS:

"check-default-import-ts": "node src/default-import.ts",
"check-named-import-ts": "node src/named-import.ts",
"check-namespace-import-ts": "node src/namespace-import.ts"

They all worked as expected.

coveralls · 2025-12-18T15:16:47Z

Pull Request Test Coverage Report for Build 20557192437

Details

36 of 39 (92.31%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.007%) to 93.867%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/index.js	36	39	92.31%

Totals
Change from base Build 20138541190:	-0.007%
Covered Lines:	949
Relevant Lines:	1011

💛 - Coveralls

Nirator78 · 2025-12-22T10:35:20Z

For me it's good to, when the pr ?

Copilot

Pull request overview

This pull request refactors the CommonJS export pattern in iconv-lite to improve TypeScript typedef compatibility. The changes align with the cjs-module-lexer compatibility patterns by switching from iconv.x = ... to exports.x = ... throughout the codebase, and updating the TypeScript definitions to use namespace-level exports with export = syntax.

Key Changes

Replaced all iconv.property assignments with exports.property throughout lib/index.js
Restructured TypeScript definitions in lib/index.d.ts from a nested object interface to namespace-level exports
Added esModuleInterop: true to tsconfig.json for better ES module/CommonJS interoperability

Reviewed changes

Copilot reviewed 1 out of 3 changed files in this pull request and generated 1 comment.

File	Description
tsconfig.json	Added `esModuleInterop: true` compiler option for improved ES module compatibility
lib/index.js	Refactored all property assignments and method definitions from `iconv.x` pattern to `exports.x` pattern for proper CommonJS exports
lib/index.d.ts	Restructured type definitions from nested object structure to namespace-level exports matching the new JavaScript export style

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-23T00:58:02Z

lib/index.js

@@ -102,7 +102,7 @@ iconv.getCodec = function getCodec (encoding) {
        //
        codec = new codecDef(codecOptions, iconv)


The variable iconv is used here but should be exports to match the refactoring throughout the rest of the file. Line 7 defines var iconv = module.exports, but since all other references have been changed to use exports directly, this reference should also be updated to exports for consistency.

Suggested change

codec = new codecDef(codecOptions, iconv)

codec = new codecDef(codecOptions, exports)

it should be module.exports or icons, imo

bjohansebas · 2025-12-23T14:23:23Z

lib/index.js

 }

-iconv.encodingExists = function encodingExists (enc) {
+exports.encodingExists = function encodingExists (enc) {


there's something that doesn't add up, and it's that technically with iconv.[nameOfObject] that's already being done, I can't understand why it doesn't work for TypeScript with the old way, but it does work with this new way, I feel like it's just a bug in attw that can't detect that rule properly

It has to do with how TypeScript transpiles the userland code.

Also, since exports. is the right way to export modules (for use in ESM), it is an okay thing to do.

We want to verify how it behaves without that option, since it is disabled by default. Additionally, it’s not an option used in DefinitelyTyped for testing, so let’s leave it disabled.

ljharb

I prefer using module.exports over exports, since that helps prevent cycles, but otherwise the JS changes look correct to me.

ljharb · 2025-12-28T17:02:18Z

lib/index.js

@@ -102,7 +102,7 @@ iconv.getCodec = function getCodec (encoding) {
        //
        codec = new codecDef(codecOptions, iconv)


it should be module.exports or icons, imo

bjohansebas

LGTM. I’ll leave it open for a bit longer so more people can take a look, and I’ll try to release it in January

plbstl added 6 commits December 14, 2025 15:00

refactor: set exported properties and methods in the exports objects

28cb4c1

typedefs: improve JSDocs for iconv options

c375614

typedefs: improve typings for Codec and enableStreamingAPI

5cf232b

typedefs: add typings for Codec.bomAware and iconv.skipDecodeWarning

60e2674

typedefs: simplify iconv namespace

0d625bc

style: lint fix

519fd3b

plbstl added 2 commits December 18, 2025 12:53

ensure tsc errors if imports fail at runtime

918326b

https://www.typescriptlang.org/tsconfig/#esModuleInterop

set esModuleInterop to true in tsconfig to allow default import o…

f19aea8

…f `iconv` in `test/types/iconv.ts`

bjohansebas requested a review from Copilot December 23, 2025 00:54

Copilot started reviewing on behalf of bjohansebas December 23, 2025 00:55 View session

Copilot AI reviewed Dec 23, 2025

View reviewed changes

bjohansebas reviewed Dec 23, 2025

View reviewed changes

typedefs: dont use of esModuleInterop

24185a7

We want to verify how it behaves without that option, since it is disabled by default. Additionally, it’s not an option used in DefinitelyTyped for testing, so let’s leave it disabled.

bjohansebas added a commit that referenced this pull request Dec 28, 2025

chages of #366 withou modific module export

44409b9

ljharb approved these changes Dec 28, 2025

View reviewed changes

typedefs: unify exports to module.exports for consistency

d3a8f85

bjohansebas approved these changes Dec 28, 2025

View reviewed changes

This was referenced Dec 28, 2025

fix(types): fix imports for type definitions #364

Closed

types: refactor Iconv type definition and add namespace export #365

Closed

bjohansebas added typescript v0 semver-patch labels Dec 28, 2025

bjohansebas self-assigned this Dec 28, 2025

bjohansebas requested review from Phillip9587 and efekrskl December 28, 2025 17:48

efekrskl approved these changes Dec 28, 2025

View reviewed changes

bjohansebas mentioned this pull request Dec 29, 2025

utf-8 is incorrect when used in browsers or React Native #369

Open

bjohansebas merged commit 7dff5ce into pillarjs:master Jan 5, 2026
59 checks passed

bjohansebas mentioned this pull request Jan 5, 2026

release: 0.7.2 #379

Merged

		@@ -102,7 +102,7 @@ iconv.getCodec = function getCodec (encoding) {
		//
		codec = new codecDef(codecOptions, iconv)

	codec = new codecDef(codecOptions, iconv)
	codec = new codecDef(codecOptions, exports)

Uh oh!

Conversation

plbstl commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbstl commented Dec 18, 2025

Uh oh!

kyota-fujikura commented Dec 18, 2025

Uh oh!

plbstl commented Dec 18, 2025

Uh oh!

coveralls commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 20557192437

Details

💛 - Coveralls

Uh oh!

Nirator78 commented Dec 22, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

ljharb Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

bjohansebas Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

plbstl Dec 26, 2025

Choose a reason for hiding this comment

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

ljharb Dec 28, 2025

Choose a reason for hiding this comment

Uh oh!

bjohansebas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

plbstl commented Dec 18, 2025 •

edited

Loading

coveralls commented Dec 18, 2025 •

edited

Loading