Move to ESM #26

msabramo · 2021-09-13T22:51:15Z

as described at https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

which was referenced at: #20 (comment)

🎉

sindresorhus · 2021-09-14T03:15:21Z

I think it's better to do this after #16. Then you don't have a chance of conflicting and we can also upgrade dependencies and not need to ignore XO rules.

msabramo · 2021-09-14T06:56:10Z

I changed this to a draft PR, so it doesn't accidentally get merged. I can resume this after #16 is done, like you said.

sindresorhus · 2021-09-14T14:37:02Z

Can you rebase from main and fix the merge conflict?

Qix- · 2021-09-14T15:03:08Z

Since we only seem to do squashes I went ahead and used the github resolver. It was just a small YAML change.

Qix- · 2021-09-14T15:04:54Z

test.js

@@ -2,7 +2,6 @@ import test from 'ava';
 import chalk from 'chalk';
 import execa from 'execa';

-process.env.FORCE_COLOR = true;


Curious, what's the reason for removing this?

Oh good question. I think it's not necessary anymore, after adding 6458c59:

--- a/package.json +++ b/package.json @@ -16,7 +16,7 @@ "node": ">=6" }, "scripts": { - "test": "xo && ava" + "test": "xo && FORCE_COLOR=1 ava" }, "files": [ "cli.js"

Basically setting process.env.FORCE_COLOR wasn't really doing anything anymore, because it happens after the import of chalk and that's too late to affect the determination of whether stdout supports color or not. For that reason, I moved it to package.json.

Qix- · 2021-09-14T15:06:02Z

package.json

+		"rules": {
+			"comma-dangle": ["warn", "only-multiline"],
+			"promise/prefer-await-to-then": "warn",
+			"arrow-body-style": "warn"


Not sure what @sindresorhus's stance is but I don't like warnings these days, I generally prefer errors or nothing at all.

Thoughts?

The rules should be followed instead of ignored.

So then these should be "error", right?

Ah okay! Yeah I'd rather follow the rules than use warnings as well. The warnings were because I started working on this before #16 was merged and I was trying not to make too many changes that could cause conflicts. Now that #16 is merged, that is no longer a concern and I can change this to a more pure approach.

Done. Changed the warnings to errors. Now there are 2 errors. Before fixing the errors, I'll quickly point out that xo and GitHub Actions `CI are doing their jobs to give us feedback to improve the code.

as described at https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c which was referenced at: chalk#20 (comment) Fixes: chalkGH-25

We need node >= 12 for ESM support.

Requires ESM.

I made a bunch `"warn"` instead of `"error"`, so I don't have to make so many code changes right now, because I don't want to risk creating conflicts with any ongoing work, such as chalkGH-16.

by: 1. using `await` on `getStdin()` instead of `then`. 2. wrapping top-level code in `async function main()` The top-level `await` seemed to *work* okay (at least for me in Node 14.17.5). However, I see 2 problems with using it. 1. According to [Top-level await is available in Node.js modules][top-level-await-in-node]: > Starting with **Node.js v14.8**, top-level await is available > (without the use of the `--harmony-top-level-await` command line flag). So it seems like this will be a pain for folks using older Node versions. 2. It seems xo doesn't like this: ``` $ npm run test > chalk-cli@4.1.0 test > xo && FORCE_COLOR=1 ava cli.js:140:15 ✖ 140:15 Parsing error: Cannot use keyword await outside an async function 1 error ``` In light of these 2 things, I decided to just wrap all the top level code in an `async` function and call it a day, but let me know if you have other ideas on how to solve these issues. [top-level-await-in-node]: https://www.stefanjudis.com/today-i-learned/top-level-await-is-available-in-node-js-modules/#top-level-%60await%60-is-available-%22unflagged%22-in-node.js-since-%60v14.8%60

Some tooling would prefer this to be a URI with a scheme and it's rendering with a squiggly in VS code and I see no reason why not to have a URL with a scheme.

Update `"repository"` field in `package.json` to one of the recommended forms described at https://docs.npmjs.com/cli/v7/configuring-npm/package-json#repository

msabramo · 2021-09-14T16:33:08Z

OK, I made the following changes:

Changed xo warnings to errors.
Fixed errors reported by xo.
Cherry-picked minor package.json metadata changes from package.json: Make author.url a URL #24 (closed).

Tests are passing!

Let me know if other changes are desired!

Extract `processDataFromArgs` & `processDataFromStdin` funcs. This avoids top-level `await`, which seems problematic. It also breaks up a large piece of code with lots of nesting into two simpler functions and at least partially addresses chalk#31

Qix-

Github doesn't allow me to make comments on "unrelated" parts of a diff, frustratingly.

@sindresorhus I noticed you don't move files to .mjs. This would remove the need to have async function main() would it not? Since .mjs allows root level awaits. I noticed you don't do this in your other packages, though.

cli.js

Qix-

Ugh github is annoying me so much today.

Qix- · 2021-09-14T16:49:03Z

cli.js

+	const dataFromStdin = await getStdin();
+	init(dataFromStdin);


Suggested change

const dataFromStdin = await getStdin();

init(dataFromStdin);

init(await getStdin());

Thanks! Fixed in f31fe1b.

Qix- · 2021-09-14T16:50:32Z

This avoids top-level await, which seems problematic.

Why?

msabramo · 2021-09-14T16:55:37Z

@qix:

This avoids top-level await, which seems problematic.

Why?

See 398bd39.

TL;DR:

Top-level await isn't supported without passing ugly flags to node until Node 14.8+.
xo emits warnings about the top-level await. Maybe eslint hasn't caught up with the latest Node.js gizmos?

Though I don't think these two points matter a ton now, after I added 4533221. That commit made it so instead of having a perhaps arguably gratuitous main function, we have two smaller functions that not only avoid the top-level await but they also modularize the code in a nice way.

msabramo · 2021-09-14T17:01:17Z

@qix:

This would remove the need to have async function main()

Note that I removed the async function main() in 4533221.

@qix

as suggested by @qix in chalk#26 (comment)

Qix- · 2021-09-14T17:38:21Z

The naked calls to async functions will result in unhandled promise rejection errors if they fail for some reason. Unless node has changed something and I haven't caught up, those calls will need to have .catch() clauses on them to properly show stack traces.

sindresorhus · 2021-09-14T18:38:09Z

xo emits warnings about the top-level await. Maybe eslint hasn't caught up with the latest Node.js gizmos?

Correct

sindresorhus · 2021-09-14T18:39:08Z

The naked calls to async functions will result in unhandled promise rejection errors if they fail for some reason. Unless node has changed something and I haven't caught up, those calls will need to have .catch() clauses on them to properly show stack traces.

meow handles that for us.

msabramo · 2021-09-14T18:50:41Z

🎉

msabramo · 2021-09-14T18:51:49Z

Is there a CHANGELOG.md or such where we should be adding these big new features like -n, the move to ESM, etc.?

sindresorhus · 2021-09-14T18:56:30Z

Thanks for your help getting this done :)

https://github.com/chalk/chalk-cli/releases/tag/v5.0.0

This was referenced Sep 13, 2021

Better error for unknown flags #20

Merged

Fix failing tests #28

Merged

msabramo force-pushed the esm branch from ae7ef5d to e89d7cf Compare September 13, 2021 23:32

msabramo marked this pull request as draft September 14, 2021 04:53

sindresorhus marked this pull request as ready for review September 14, 2021 14:36

Qix- reviewed Sep 14, 2021

View reviewed changes

msabramo added 4 commits September 14, 2021 08:37

Move to ESM

7f28762

as described at https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c which was referenced at: chalk#20 (comment) Fixes: chalkGH-25

workflows/main.yml: Remove node-version 10

f2dfe18

We need node >= 12 for ESM support.

Upgrade xo to latest

1d2607c

Requires ESM.

Fix xo errors

13fec1c

I made a bunch `"warn"` instead of `"error"`, so I don't have to make so many code changes right now, because I don't want to risk creating conflicts with any ongoing work, such as chalkGH-16.

msabramo force-pushed the esm branch from 48253d6 to 13fec1c Compare September 14, 2021 15:37

msabramo added 5 commits September 14, 2021 08:39

package.json: Change xo warnings to errors

5efb63d

Fix arrow-body-style error in test.js

ba2e82f

package.json: Make author.url a URL

8c36c6e

Some tooling would prefer this to be a URI with a scheme and it's rendering with a squiggly in VS code and I see no reason why not to have a URL with a scheme.

package.json: Update repository to recommend form

fc9bec3

Update `"repository"` field in `package.json` to one of the recommended forms described at https://docs.npmjs.com/cli/v7/configuring-npm/package-json#repository

Qix- requested changes Sep 14, 2021

View reviewed changes

cli.js Outdated Show resolved Hide resolved

Qix- requested changes Sep 14, 2021

View reviewed changes

init(await getStdin());

f31fe1b

as suggested by @qix in chalk#26 (comment)

msabramo requested review from Qix- and sindresorhus September 14, 2021 17:22

sindresorhus merged commit a3efcb5 into chalk:main Sep 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to ESM #26

Move to ESM #26

msabramo commented Sep 13, 2021 •

edited

Loading

sindresorhus commented Sep 14, 2021

msabramo commented Sep 14, 2021

sindresorhus commented Sep 14, 2021

Qix- commented Sep 14, 2021

Qix- Sep 14, 2021

msabramo Sep 14, 2021 •

edited

Loading

Qix- Sep 14, 2021

sindresorhus Sep 14, 2021

Qix- Sep 14, 2021 •

edited

Loading

msabramo Sep 14, 2021

msabramo Sep 14, 2021

msabramo commented Sep 14, 2021 •

edited

Loading

Qix- left a comment

Qix- left a comment

Qix- Sep 14, 2021

msabramo Sep 14, 2021

Qix- commented Sep 14, 2021

msabramo commented Sep 14, 2021 •

edited

Loading

msabramo commented Sep 14, 2021 •

edited

Loading

Qix- commented Sep 14, 2021

sindresorhus commented Sep 14, 2021

sindresorhus commented Sep 14, 2021

msabramo commented Sep 14, 2021

msabramo commented Sep 14, 2021

sindresorhus commented Sep 14, 2021

	const dataFromStdin = await getStdin();
	init(dataFromStdin);
	init(await getStdin());

Move to ESM #26

Move to ESM #26

Conversation

msabramo commented Sep 13, 2021 • edited Loading

sindresorhus commented Sep 14, 2021

msabramo commented Sep 14, 2021

sindresorhus commented Sep 14, 2021

Qix- commented Sep 14, 2021

Qix- Sep 14, 2021

Choose a reason for hiding this comment

msabramo Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Qix- Sep 14, 2021

Choose a reason for hiding this comment

sindresorhus Sep 14, 2021

Choose a reason for hiding this comment

Qix- Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

msabramo Sep 14, 2021

Choose a reason for hiding this comment

msabramo Sep 14, 2021

Choose a reason for hiding this comment

msabramo commented Sep 14, 2021 • edited Loading

Qix- left a comment

Choose a reason for hiding this comment

Qix- left a comment

Choose a reason for hiding this comment

Qix- Sep 14, 2021

Choose a reason for hiding this comment

msabramo Sep 14, 2021

Choose a reason for hiding this comment

Qix- commented Sep 14, 2021

msabramo commented Sep 14, 2021 • edited Loading

msabramo commented Sep 14, 2021 • edited Loading

Qix- commented Sep 14, 2021

sindresorhus commented Sep 14, 2021

sindresorhus commented Sep 14, 2021

msabramo commented Sep 14, 2021

msabramo commented Sep 14, 2021

sindresorhus commented Sep 14, 2021

msabramo commented Sep 13, 2021 •

edited

Loading

msabramo Sep 14, 2021 •

edited

Loading

Qix- Sep 14, 2021 •

edited

Loading

msabramo commented Sep 14, 2021 •

edited

Loading

msabramo commented Sep 14, 2021 •

edited

Loading

msabramo commented Sep 14, 2021 •

edited

Loading