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.

Sign up for GitHub

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

Jump to bottom

fix: make xo happy #145

Merged
merged 7 commits into from
Dec 13, 2022
Merged

fix: make xo happy #145

merged 7 commits into from
Dec 13, 2022

Conversation

idranme
Copy link
Contributor

@idranme idranme commented Dec 7, 2022 •
edited
Loading

fix:

  index.d.ts:1:1
  ✖   1:1   All imports in the declaration are only used as types. Use import type.  @typescript-eslint/consistent-type-imports
  ✖   3:18  Use a type instead of an interface.                                      @typescript-eslint/consistent-type-definitions
  ✖  24:18  Use a type instead of an interface.                                      @typescript-eslint/consistent-type-definitions

  index.test-d.ts:3:1
  ✖   3:1   Imports "Options" and "ColorInfo" are only used as types.                @typescript-eslint/consistent-type-imports

from https://github.com/chalk/chalk/actions/runs/3638940747/jobs/6142101330

Error: /home/runner/work/chalk/chalk/source/vendor/supports-color/browser.js: line 6, col 12, Error - Optional chainings are not supported until Node.js 14.0.0. The configured version range is '^12.17.0 || ^14.13 || >=16.0.0'. (n/no-unsupported-features/es-syntax)
Error: /home/runner/work/chalk/chalk/source/vendor/supports-color/index.js: line 6, col 46, Error - Optional chainings are not supported until Node.js 14.0.0. The configured version range is '^12.17.0 || ^14.13 || >=16.0.0'. (n/no-unsupported-features/es-syntax)
Error: /home/runner/work/chalk/chalk/source/vendor/supports-color/index.js: line 6, col 53, Error - Nullish coalescing operators are not supported until Node.js 14.0.0. The configured version range is '^12.17.0 || ^14.13 || >=16.0.0'. (n/no-unsupported-features/es-syntax)
Error: /home/runner/work/chalk/chalk/source/vendor/supports-color/index.js: line 141, col 4, Error - Missing braces in case clause. (unicorn/switch-case-braces)
Error: /home/runner/work/chalk/chalk/source/vendor/supports-color/index.js: line 143, col 4, Error - Missing braces in case clause. (unicorn/switch-case-braces)

@sindresorhus
Copy link
Member

You need to update XO for this. Please also always run npm test before submitting a pull request.

@idranme idranme mentioned this pull request Dec 7, 2022
Merged
@idranme idranme changed the title fix: missing braces in case clause fix: make xo happy Dec 7, 2022
@idranme
Copy link
Contributor Author

idranme commented Dec 7, 2022

You need to update XO for this. Please also always run npm test before submitting a pull request.

done

Qix-
Qix- reviewed Dec 7, 2022
View reviewed changes
index.d.ts Outdated
@@ -21,7 +21,7 @@ export type ColorSupportLevel = 0 | 1 | 2 | 3;
/**
Detect whether the terminal supports color.
*/
export interface ColorSupport {
export type ColorSupport = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the same. What was the XO error message about?

Copy link
Contributor Author

@idranme idranme Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the same. What was the XO error message about?

Use a type instead of an interface. @typescript-eslint/consistent-type-definitions

browser.js Outdated
@@ -3,7 +3,7 @@
const level = (() => {
if (navigator.userAgentData) {
const brand = navigator.userAgentData.brands.find(({brand}) => brand === 'Chromium');
if (brand?.version > 93) {
if (brand && brand.version > 93) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this fixes or improves anything, what was the XO error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this fixes or improves anything, what was the XO error?

Error - Optional chainings are not supported until Node.js 14.0.0. The configured version range is '^12.17.0 || ^14.13 || >=16.0.0'. (n/no-unsupported-features/es-syntax)

index.js Outdated
@@ -3,7 +3,7 @@ import os from 'node:os';
import tty from 'node:tty';

// From: https://github.com/sindresorhus/has-flag/blob/main/index.js
function hasFlag(flag, argv = globalThis.Deno?.args ?? process.argv) {
function hasFlag(flag, argv = globalThis.Deno ? globalThis.Deno.args : process.argv) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this as an improvement. What was the XO error?

Copy link
Contributor Author

@idranme idranme Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this as an improvement. What was the XO error?

line 6, col 46, Error - Optional chainings are not supported until Node.js 14.0.0. The configured version range is '^12.17.0 || ^14.13 || >=16.0.0'. (n/no-unsupported-features/es-syntax)
line 6, col 53, Error - Nullish coalescing operators are not supported until Node.js 14.0.0. The configured version range is '^12.17.0 || ^14.13 || >=16.0.0'. (n/no-unsupported-features/es-syntax)

index.js
@@ -141,6 +141,7 @@ function _supportsColor(haveStream, {streamIsTTY, sniffFlags = true} = {}) {
case 'iTerm.app': {
return version >= 3 ? 3 : 2;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this an error?

Error - Missing braces in case clause. (unicorn/switch-case-braces)

@idranme
Copy link
Contributor Author

idranme commented Dec 7, 2022 •
edited
Loading

Maybe we can try using esbuild.
Or drop Node 12.

@idranme idranme requested a review from Qix- December 8, 2022 11:09
sindresorhus
sindresorhus reviewed Dec 12, 2022
View reviewed changes
index.test-d.ts Outdated
Comment on lines 3 to 4
import type {Options, ColorInfo} from './index.d';
import supportsColor, {createSupportsColor} from './index.js';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a single import

@sindresorhus
Copy link
Member

Just downgrade XO to the last version that supports Node.js 12. I don't want to do a major release right now. I prefer to wait a few months until we can target Node.js 16.

@idranme
Copy link
Contributor Author

idranme commented Dec 13, 2022

Just downgrade XO to the last version that supports Node.js 12. I don't want to do a major release right now. I prefer to wait a few months until we can target Node.js 16.

done

@idranme idranme requested review from sindresorhus and Qix- and removed request for Qix- and sindresorhus December 13, 2022 01:34
@sindresorhus sindresorhus merged commit 925035b into chalk:main Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Qix- Qix- Qix- left review comments

@sindresorhus sindresorhus Awaiting requested review from sindresorhus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@idranme @sindresorhus @Qix- @idanran