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

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

Open
zurmokeeper opened this issue Mar 7, 2022 · 5 comments

Comments

@zurmokeeper
Copy link

zurmokeeper commented Mar 7, 2022

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

ashtuchkin commented Mar 7, 2022

if it is not Buffer, throw error

it might break legacy code

@zurmokeeper
Copy link
Author

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
Copy link
Owner

This will unfortunately break legacy code that is passing strings.

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

No branches or pull requests

2 participants