Skip to content

Conversation

@richiemccoll
Copy link

@richiemccoll richiemccoll commented Oct 30, 2025

Related issue

Fixes #198

Test cases

  • basic
  • advanced modifiers
  • bg styles
  • chained styles
  • complex chaining
  • commonjs require
  • ESM dynamic import
  • mixed imports and usage
  • warn when un-migratable chalk api is found
  • different importing methode
    • const mychalk = require('chalk');
    • const { red }. = require(-'chalk'); maybe hard
    • const { red; foo }. = require(-"chalk");
    • const foo = await import('chalk');
    • const { red } = await import('chalk')
    • import * as chalk from 'chalk'
    • import { red } from 'chalk';
    • import { red as colorRed 'chalk';
  • dependency removal(?)

@AugustinMauroy
Copy link
Member

AugustinMauroy commented Oct 30, 2025

don't miss to do a npm I at root of the repo to sync the lock file 😁
Also you can use multiple step codemod to separate job if needed like for https://github.com/nodejs/userland-migrations/tree/main/recipes/node-url-to-whatwg-url

For dependency removal I'm asking the Codemod team. because on V0 when the tools was JS only and there was a simple thing to manage dep

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

some code style

@avivkeller
Copy link
Member

@AugustinMauroy the PR is still a draft, no need to nitpick yet

@AugustinMauroy
Copy link
Member

AugustinMauroy commented Oct 30, 2025

@AugustinMauroy the PR is still a draft, no need to nitpick yet

😅 oops I'm flash macqueen. Nop kidding just inattentive

@richiemccoll
Copy link
Author

richiemccoll commented Oct 31, 2025

@AugustinMauroy I've pushed a few commits which I believe address some of the earlier comments. From the original Github issue, 7/8 test cases are passing. The one test case remaining is the complex chaining example.

const styles = level === "error" ? chalk.red.bold : chalk.yellow;
const styles =
	level === "error"
		? (text) => styleText(["red", "bold"], text)
		: (text) => styleText("yellow", text);

I have a rough idea/implementation locally for this, but it's a bit nasty and I'm not sure I like it.

But I'm keen to get feedback on this initial approach and understand if there are possibly better ways of implementing.

@richiemccoll richiemccoll marked this pull request as ready for review October 31, 2025 16:58
@AugustinMauroy
Copy link
Member

I'm going to review your current code.

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

WOW clean. About complex thing I didn't have any idea now I'm getting tired. But you do a great job here.

Missing tests/support about import.

  • const mychalk = require('chalk');
  • const { red }. = require(-'chalk'); maybe hard
  • const { red; foo }. = require(-"chalk");
  • const foo = await import('chalk');
  • const { red } = await import('chalk')
  • import * as chalk from 'chalk'
  • import { red } from 'chalk';
  • import { red as colorRed 'chalk';

For dependcy par you I'll need custom thing (new step on workflow.yaml):

  • step that have JSSG child_process & fs capacity enabled
  • function that use FS to detect which package manager it's use (npm, yarn, pnpm)
  • new function on package-json utility that removes dep
    So jssg step will be architecture as:
  • get the package manager
  • remove dep with the utility function
  • run the good child process

for this part I can take over the pr to help you.

https://docs.codemod.com/cli/packages/building-workflows#steps
https://docs.codemod.com/jssg/security

// Pattern 1: chalk.method(text) -> styleText("method", text)
const method = methodMatch.text();
const text = textMatch.text();
const styleMethod = COMPAT_MAP[method] || method;
Copy link
Member

Choose a reason for hiding this comment

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

what if the method is valid node style thing ?

Copy link
Author

Choose a reason for hiding this comment

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

Do you have an example of what you mean?

the reason behind COMPAT_MAP is really to handle the naming differences between chalk and styleText. I haven't done an exhaustive comparison yet, but the obvious one was overlined vs overline.

Copy link
Member

@AugustinMauroy AugustinMauroy Nov 1, 2025

Choose a reason for hiding this comment

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

my question is "Is there a chalk method that does not exist at all under node? " if yes what happened for the modified code ?

Copy link
Author

Choose a reason for hiding this comment

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

There are a few cases that are not currently handled in the PR yet.

How should we handle those?

Copy link
Member

Choose a reason for hiding this comment

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

before the initialization of styleMethod we should check if the method is hex OR rgb OR ansi256 if yes warn the user that is not migratable. And advice them to use "standard" colors

TIP root.filename() give the relative path from the cwd of the proccessed file.

For other methods idk maybe also warn but I'm not sure how to advice the user

Copy link
Author

Choose a reason for hiding this comment

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

I've added a test case for unsupported features: feat(chalk-to-util-styletext): add unsupported features test

I'm not sure how we should warn the user though in this case. We could add a TODO comment explaining why it wasn't transformed?

Copy link
Member

Choose a reason for hiding this comment

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

console.warn("${fileName}:${row}:${col} use a chalk method that didn't have any 1:1 equivalent in util.styleText please review this line")

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.

feat(chalk-to-util.styleText): handle migration

3 participants