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

build(deps): remove chalk and use styleText instead #798

Conversation

kinggoesgaming
Copy link
Contributor

@kinggoesgaming kinggoesgaming commented Jul 21, 2024

  • refactor: replace chalk with styleText
  • build(deps): remove chalk
  • style(imports): fix import order

Test Plan

No tests or docs need changing

Screenshots

No change that requires diff of screenshots

closes: #792

app/entry.server.tsx Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Super!

app/entry.server.tsx Show resolved Hide resolved
@jacobparis
Copy link
Contributor

styleText doesn't land until Node 22 (node 21.7 technically) which won't be LTS until October, so I don't think we can merge this until then based on this decision doc

https://github.com/epicweb-dev/epic-stack/blob/main/docs/decisions/021-node-version.md

@kinggoesgaming
Copy link
Contributor Author

styleText doesn't land until Node 22 (node 21.7 technically) which won't be LTS until October, so I don't think we can merge this until then based on this decision doc

https://github.com/epicweb-dev/epic-stack/blob/main/docs/decisions/021-node-version.md

I am so used to using it that I didn't realize it is still in Active development stage.

I will try to keep this PR up-to-date until it is stablized.

@kinggoesgaming
Copy link
Contributor Author

Another point: this does not actually remove chalk from the dependency tree, as other dependencies do depend on chalk, but now it is a transitive dev dependency.

Signed-off-by: Hunar Roop Kahlon <[email protected]>
Signed-off-by: Hunar Roop Kahlon <[email protected]>
@kinggoesgaming kinggoesgaming force-pushed the 792-use-styletext-from-node-util branch from 353f1d2 to 181f7e2 Compare August 3, 2024 20:19
@alcpereira
Copy link
Contributor

alcpereira commented Aug 4, 2024

styleText doesn't land until Node 22 (node 21.7 technically) which won't be LTS until October, so I don't think we can merge this until then based on this decision doc

https://github.com/epicweb-dev/epic-stack/blob/main/docs/decisions/021-node-version.md

What do you think about updating the requirement to Node 20.12.0?

I'm used to work with .nvmrc or .node-version files so that Node version manager like fnm can select the right version for the project.
Is it worth opening a PR to add one of those (with corresponding documentation)? So it might also unblock this PR.
Edit: This is already suggested in #799

@kentcdodds
Copy link
Member

Why don't node managers reference the engines config in the package.json? I'd really rather avoid adding an extra file to the root just for the node version.

@kentcdodds
Copy link
Member

I'm going to close this for now. We can revisit when Node 22 is our base.

@kentcdodds kentcdodds closed this Aug 27, 2024
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.

Use styleText from node:util
4 participants