ashtuchkin / iconv-lite

Convert character encodings in pure javascript.
MIT License
3.04k stars 282 forks source link

Problem with the type declaration of the first parameter of the decode method in index.d.ts? #294

Open zurmokeeper opened 2 years ago

zurmokeeper commented 2 years ago

Problem with the type declaration of the first parameter of the decode method in index.d.ts?

export function decode(buffer: Buffer, encoding: string, options?: Options): string;

The first parameter of decode is declared as Buffer , but it is also possible to pass in string and number, but the correct result is not obtained. This will mislead developers, especially when the string type is used, the implementation code is as follows:

iconv.decode = function decode(buf, encoding, options) {
    if (typeof buf === 'string') {
        if (!iconv.skipDecodeWarning) {
        ......
        iconv.skipDecodeWarning = true;
    }

    buf = Buffer.from("" + (buf || ""), "binary"); // Ensure buffer.
    }
    ..........................................
}

The string is processed by default binary-encoded Buffer, and there is no description in the declaration file. I hope you can modify the declaration file, for example, change it to:

/**
 * @description [Buffer | string] content If the incoming string type, it will be processed with binary encoded Buffer
 */
 export function decode(content: Buffer | string, encoding: string, options?: Options): string;

In this way, the developer will clearly know what type of parameters to pass, and also know what the decode method does by default.

ashtuchkin commented 2 years ago

Unfortunately in JS you can pass anything anywhere. It's not guaranteed it will work, especially if it's not supported by the typescript declaration. The fact that it somewhat works today is an artifact of the past and should not be relied upon. Please don't pass strings to decode method. See https://github.com/ashtuchkin/iconv-lite/wiki/Use-Buffers-when-decoding for details.

zurmokeeper commented 2 years ago

Unfortunately in JS you can pass anything anywhere. It's not guaranteed it will work, especially if it's not supported by the typescript declaration. The fact that it somewhat works today is an artifact of the past and should not be relied upon. Please don't pass strings to decode method. See https://github.com/ashtuchkin/iconv-lite/wiki/Use-Buffers-when-decoding for details.

It is true that js has this problem, so why not judge, if it is not Buffer, throw error, and then prompt the developer to only input parameters of type Buffer?

ashtuchkin commented 2 years ago

if it is not Buffer, throw error

it might break legacy code

zurmokeeper commented 2 years ago

Look carefully, add one more judgment here, it will not destroy the original code?

iconv.decode = function decode(buf, encoding, options) {

    +//Look carefully, add one more judgment here, it will not destroy the original code?
    +if(Object.prototype.call.toString(buf) != '[object Uint8Array]'){
    +    throw new TypeError("content must be a buffer")
    +}

    if (typeof buf === 'string') {
        if (!iconv.skipDecodeWarning) {
        ......
        iconv.skipDecodeWarning = true;
    }

    buf = Buffer.from("" + (buf || ""), "binary"); // Ensure buffer.
    }
    ..........................................
}
ashtuchkin commented 2 years ago

This will unfortunately break legacy code that is passing strings.