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

Rewrite with TypeScript & move to ESM #20

Closed
wants to merge 3 commits into from

Conversation

LitoMore
Copy link
Contributor

@LitoMore LitoMore commented Sep 7, 2022

Changes

  • Rewrite with TypeScript, resolves Supporting typing #15
  • Move to ESM
  • Use GitHub Actions
  • Drop .travis.yml
  • Use c8 instead of nyc
  • Bump all dependencies to latest
  • Remove supportsHyperlink.stdout and supportsHyperlink.stderr
    • Note: Users can easy implement this API, we could remove it
  • Add missing tests
    • Note: There is one uncovered branch, that requires mock process.env, but the code to execute is in supports-color

@sindresorhus
Copy link
Collaborator

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

@LitoMore
Copy link
Contributor Author

LitoMore commented Sep 8, 2022

I noticed the TypeScript definition issue chalk/supports-color#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
Copy link
Collaborator

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

@voxpelli
Copy link
Contributor

voxpelli commented Oct 7, 2022

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

LitoMore commented Oct 7, 2022

@voxpelli Wow. Thanks.

@LitoMore
Copy link
Contributor Author

LitoMore commented Oct 7, 2022

Closing in favor of #21.

@LitoMore LitoMore closed this Oct 7, 2022
@LitoMore LitoMore deleted the move-to-typescript branch October 7, 2022 17:17
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

Successfully merging this pull request may close these issues.

Supporting typing
3 participants