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

[babel 8] Remove getChalk function #15812

Merged
merged 10 commits into from Jul 24, 2023
Merged

[babel 8] Remove getChalk function #15812

merged 10 commits into from Jul 24, 2023

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jul 21, 2023 •
edited by JLHwung

Q                       A
Fixed Issues? Fixes #1, Fixes #2
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link babel/website#2765
Any Dependency Changes?
License MIT

There is no good reason for us to have chalk exposed as part of our public API, and it's making the upgrade to v4/5 incredibly complex.

@babel-bot
Copy link
Collaborator

babel-bot commented Jul 21, 2023 •
edited

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/54949/

@JLHwung
Copy link
Contributor

JLHwung commented Jul 21, 2023

I guess getChalk was there because the code-frame test wants to manipulate the Chalk instance used by the highlight package. If this API is removed we should resolve chalk from the highlight package.

@nicolo-ribaudo
Copy link
Member Author

Yeah thanks, I was just reading #15499 (comment) some minutes ago :)

@nicolo-ribaudo
Copy link
Member Author

image

Oh no, we miss chalk 3!

@nicolo-ribaudo nicolo-ribaudo marked this pull request as draft July 21, 2023 17:19
@nicolo-ribaudo nicolo-ribaudo marked this pull request as ready for review July 23, 2023 09:16
@nicolo-ribaudo
Copy link
Member Author

CI is finally green

nicolo-ribaudo
nicolo-ribaudo commented Jul 23, 2023
View reviewed changes
packages/babel-highlight/test/index.js Outdated Show resolved Hide resolved
packages/babel-highlight/test/index.js Outdated Show resolved Hide resolved
JLHwung
JLHwung approved these changes Jul 24, 2023
View reviewed changes
packages/babel-highlight/src/index.ts
let chalkWithForcedColor: Chalk = undefined;
function getChalk(forceColor: boolean) {
if (forceColor) {
chalkWithForcedColor ??= new chalk.constructor({ enabled: true, level: 1 });
Copy link
Contributor

Choose a reason for hiding this comment

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

If forceColor is true, getChalk will return a memoized chalk instance, previously it returned a new instance. The new behaviour aligns with the one when forceColor is false.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I noticed it while changing this -- I think it's ok, it has the benefit of allocating less objects (+ as you noted aligning with forceColor: false) and no actual disadvantages.

@JLHwung
Copy link
Contributor

JLHwung commented Jul 24, 2023

Can you update babel/website#2765?

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Jul 24, 2023
Merged
@nicolo-ribaudo nicolo-ribaudo merged commit 7a3e18a into babel:main Jul 24, 2023
57 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the remove-getchalk branch July 24, 2023 13:32
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Jul 24, 2023
Merged
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Aug 2, 2023
Draft
4 tasks
@JLHwung JLHwung added PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release and removed babel 8 labels Aug 9, 2023
@JLHwung JLHwung added this to the v8.0.0 milestone Aug 9, 2023
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Nov 9, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JLHwung JLHwung JLHwung approved these changes

@liuxingbaoyu liuxingbaoyu liuxingbaoyu approved these changes

Assignees
No one assigned
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release
Projects
None yet
Milestone
v8.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nicolo-ribaudo @babel-bot @JLHwung @liuxingbaoyu