Skip to content

[babel 8] Remove getChalk function#15812

Merged
nicolo-ribaudo merged 10 commits into
babel:mainfrom
nicolo-ribaudo:remove-getchalk
Jul 24, 2023
Merged

[babel 8] Remove getChalk function#15812
nicolo-ribaudo merged 10 commits into
babel:mainfrom
nicolo-ribaudo:remove-getchalk

Conversation

@nicolo-ribaudo

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

Copy link
Copy Markdown
Member
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

babel-bot commented Jul 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

@JLHwung

JLHwung commented Jul 21, 2023

Copy link
Copy Markdown
Contributor

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

nicolo-ribaudo commented Jul 21, 2023

Copy link
Copy Markdown
Member Author

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

@nicolo-ribaudo

nicolo-ribaudo commented Jul 21, 2023

Copy link
Copy Markdown
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 commented Jul 21, 2023
View reviewed changes
Comment thread packages/babel-highlight/test/index.js Outdated
nicolo-ribaudo
nicolo-ribaudo commented Jul 22, 2023
View reviewed changes
Comment thread packages/babel-highlight/test/index.js
nicolo-ribaudo
nicolo-ribaudo commented Jul 22, 2023
View reviewed changes
Comment thread packages/babel-highlight/test/index.js Outdated
@nicolo-ribaudo
nicolo-ribaudo marked this pull request as ready for review July 23, 2023 09:16
@nicolo-ribaudo

nicolo-ribaudo commented Jul 23, 2023

Copy link
Copy Markdown
Member Author

CI is finally green

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

@JLHwung JLHwung Jul 24, 2023

Copy link
Copy Markdown
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.

@nicolo-ribaudo nicolo-ribaudo Jul 24, 2023

Copy link
Copy Markdown
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

JLHwung commented Jul 24, 2023

Copy link
Copy Markdown
Contributor

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
@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
Merged
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.

4 participants

@nicolo-ribaudo @babel-bot @JLHwung @liuxingbaoyu