jamestalmage / supports-hyperlinks

Detect whether a terminal emulator supports hyperlinks
MIT License
54 stars 13 forks source link

Rewrite with TypeScript & move to ESM #20

Closed LitoMore closed 1 year ago

LitoMore commented 1 year ago

Changes

sindresorhus commented 1 year ago

I don't really see the point of rewriting such a simple package in TS.

LitoMore commented 1 year ago

I noticed the TypeScript definition issue https://github.com/chalk/supports-color/pull/138 while writing this pull request.

I actually solved some potential problems while I was writing this pull request.

For example the parseVersion function:

function parseVersion(versionString) {
    if (/^\d{3,4}$/.test(versionString)) {
        // Env var doesn't always use dots. example: 4601 => 46.1.0
        const m = /(\d{1,2})(\d{2})/.exec(versionString);
        return {
            major: 0,
            minor: parseInt(m[1], 10),
            patch: parseInt(m[2], 10)
        };
    }

    const versions = (versionString || '').split('.').map(n => parseInt(n, 10));
    return {
        major: versions[0],
        minor: versions[1],
        patch: versions[2]
    };
}

We are accessing its regex result with m[1], the m is possibly null.

Although we have /^\d{3,4}$/.test(versionString) condition to make sure the /(\d{1,2})(\d{2})/.exec(versionString) always have result. But someone else might inadvertently make things go wrong here.

TypeScript can help us to avoid lots of theses kinds of errors.

But if you insist you don't need TypeScript, I can switch back to JavaScript. That's up to you.

sindresorhus commented 1 year ago

Let's see what James thinks. It's his repo after all.

voxpelli commented 1 year ago

I made a simpler alternative in #21 as I agree with @sindresorhus that TS is overkill for a module like this and simply adding some JSDoc comments can achieve full strict typing with existing code.

LitoMore commented 1 year ago

@voxpelli Wow. Thanks.

LitoMore commented 1 year ago

Closing in favor of #21.