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

refactor (parseCssColor): use parseInt, avoid unnecessary coercion #20856

Merged
merged 4 commits into from
Nov 30, 2023

Conversation

ljharb
Copy link
Contributor

@ljharb ljharb commented Oct 9, 2023

Upstream some changes from nodejs/node#49205

@CLAassistant
Copy link

CLAassistant commented Oct 9, 2023

CLA assistant check
All committers have signed the CLA.

@ljharb ljharb changed the title parseCssColor: use parseInt, avoid unnecessary coercion refactor (parseCssColor): use parseInt, avoid unnecessary coercion Oct 9, 2023
@ljharb ljharb closed this Oct 9, 2023
@ljharb ljharb reopened this Oct 9, 2023
@bartlomieju
Copy link
Member

Thanks for the PR @ljharb; any chance you could update cli/tests/node_compat/test/parallel/test-util-format.js to include the tests from PR in Node repo?

@ljharb
Copy link
Contributor Author

ljharb commented Oct 9, 2023

Sure, i'll have to clone the repo so give me a few days.

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -2742,34 +2742,34 @@ const HSL_PATTERN = new SafeRegExp(
);

function parseCssColor(colorString) {
if (MapPrototypeHas(colorKeywords, colorString)) {
colorString = MapPrototypeGet(colorKeywords, colorString);
if (colorKeywords.has(colorString)) {
Copy link
Member

@lucacasonato lucacasonato Oct 10, 2023

Choose a reason for hiding this comment

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

FYI to other reviewers: safe because SafeMap, but may result in prefer-primordials linter errors. Let's see :)

EDIT: linter is happy :)

@lucacasonato
Copy link
Member

@bartlomieju NO_COLOR seems to be enabled in the node compat tests, so the tests don't pass. PTAL

@ljharb
Copy link
Contributor Author

ljharb commented Oct 10, 2023

@bartlomieju actually these are just refactors and have nothing to do with the meat of the node PR; what tests would you want me to include?

@bartlomieju
Copy link
Member

@ljharb, oh I thought these tests were checking this change:

assert.strictEqual(
  util.formatWithOptions({ colors: true }, '%cfoo', 'color: red'),
  '\x1b[31mfoo\x1b[0m'
);

assert.strictEqual(
  util.formatWithOptions(
    { colors: true },
    '%cfoo',
    'color: red; background-color: blue'
  ),
  '\x1B[44m\x1B[31mfoo\x1B[0m'
);

assert.strictEqual(
  util.formatWithOptions({ colors: true }, '%cfoo', 'color: red', 'bar'),
  '\x1b[31mfoo\x1b[0m bar'
);

assert.strictEqual(
  util.formatWithOptions({ colors: true }, '%cfoo%c bar', 'color: red', ''),
  '\x1b[31mfoo\x1b[39m bar\x1b[0m'
);

assert.strictEqual(
  util.formatWithOptions(
    { colors: true },
    '%cfoo %cbar',
    'color: red',
    'color: blue'
  ),
  '\x1b[31mfoo \x1b[34mbar\x1b[0m'
);

I'll take a look at the CI failure.

ljharb and others added 2 commits November 29, 2023 12:38
Upstream some changes from nodejs/node#49205

Signed-off-by: Jordan Harband <ljharb@gmail.com>
@ljharb
Copy link
Contributor Author

ljharb commented Nov 29, 2023

any update? These are just simple refactors to improve the code quality and potentially performance.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Sorry for a slow response @ljharb. I revert that test file change as running update scripts for Node compat tests causes them to be removed anyway. Thanks, LGTM

@bartlomieju bartlomieju enabled auto-merge (squash) November 30, 2023 18:14
@bartlomieju bartlomieju merged commit 334c118 into denoland:main Nov 30, 2023
@ljharb ljharb deleted the patch-1 branch November 30, 2023 18:40
@ljharb
Copy link
Contributor Author

ljharb commented Nov 30, 2023

Thanks!

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.

4 participants