GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: add a condition if the argument of `DomainToUnicode` is empty by pluris · Pull Request #49097 · nodejs/node

Conversation

@pluris

@pluris pluris commented Aug 10, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Add an empty check for the input, like DomainToASCII in the line below.

node/src/node_url.cc

Lines 79 to 82 in f1b3ade

std::string input = Utf8Value(env->isolate(), args[0]).ToString();
if (input.empty()) {
return args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(env->isolate(), ""));
}

Refs : #46410

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Aug 10, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/url
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Aug 10, 2023
anonrig
anonrig reviewed Aug 10, 2023
View reviewed changes

@anonrig anonrig left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

What's the goal of this pull request? Can you provide an example and update your PR with a test?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pluris

pluris commented Aug 11, 2023

Copy link
Copy Markdown
Contributor Author

@anonrig Thank you for your comments.
I referred to the IDNA Mapping Table from the WhatWG URL.
I checked the handling of empty strings,
I saw that DomainToASCII and DomainToUnicode should be treated the same.
Therefore, in my opinion, it is in DomainToASCII, but it seems to be missing in DomainToUnicode, so I added it.

  • IDNA Mapping Table : https://www.unicode.org/reports/tr46/#TableDerivationStep6
  • DomainToASCII : https://www.unicode.org/reports/tr46/#ToUnicode
  • DomainToUnicode : https://www.unicode.org/reports/tr46/#ToASCII
  • WhatWG URL : https://url.spec.whatwg.org/
    The same considerations as with domain-to-ASCII apply.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig

anonrig commented Aug 11, 2023

Copy link
Copy Markdown
Member

@pluris It already returns an empty string when an empty string is passed. It seems like this is an optimization for performance for the empty input to perform faster.

Welcome to Node.js v20.5.0.
Type ".help" for more information.
> require('url').domainToUnicode('')
''
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig reviewed Aug 11, 2023
View reviewed changes
Comment thread src/node_url.cc Outdated
Comment on lines 104 to 105

@anonrig anonrig Aug 11, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
return args.GetReturnValue().Set(
String::NewFromUtf8(env->isolate(), "").ToLocalChecked());
return args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(env->isolate(), ""));

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pluris reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@anonrig anonrig Aug 18, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you apply the changes?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pluris reacted with eyes emoji
All reactions
  • 👀 1 reaction

@pluris pluris Aug 26, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@anonrig Oh, I'm so sorry. I checked this comment too late. I thought I fixed it, but maybe I didn't add a commit...
It seems that the PR has been merged, but I will fix it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pluris

pluris commented Aug 11, 2023

Copy link
Copy Markdown
Contributor Author

@anonrig
If so, would it be better to do the same by removing the line below?

node/src/node_url.cc

Lines 79 to 82 in f1b3ade

std::string input = Utf8Value(env->isolate(), args[0]).ToString();
if (input.empty()) {
return args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(env->isolate(), ""));
}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig

anonrig commented Aug 11, 2023

Copy link
Copy Markdown
Member

@pluris I don't think removing it would make a difference since it is a single branch (in c++). So, personally I'm fine with adding it to domainToUnicode as well.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pluris

pluris commented Aug 11, 2023

Copy link
Copy Markdown
Contributor Author

@anonrig Thank you for your opinion. It helped me a lot. 😄
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 11, 2023
anonrig
anonrig approved these changes Aug 11, 2023
View reviewed changes
@pluris pluris closed this Aug 11, 2023
@pluris pluris reopened this Aug 11, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 11, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 11, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53205/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@anonrig

anonrig commented Aug 11, 2023

Copy link
Copy Markdown
Member

@pluris Can you rebase from main and force-push?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pluris
pluris force-pushed the fix/check_empty_input branch from e2fd13c to 4e380fd Compare August 12, 2023 00:53
@pluris

pluris commented Aug 12, 2023

Copy link
Copy Markdown
Contributor Author

@anonrig Yes, I did rebase and force-push.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig approved these changes Aug 12, 2023
View reviewed changes
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 12, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 12, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 12, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53216/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Aug 12, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53253/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Aug 13, 2023
CI Reliability 2023-08-13 nodejs/reliability#635
Open
CI Reliability 2023-08-14 nodejs/reliability#636
Open
CI Reliability 2023-08-15 nodejs/reliability#637
Open
@nodejs-github-bot

nodejs-github-bot commented Aug 16, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53379/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Aug 17, 2023
CI Reliability 2023-08-17 nodejs/reliability#639
Open
26 tasks
@nodejs-github-bot

nodejs-github-bot commented Aug 17, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53402/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Aug 18, 2023
CI Reliability 2023-08-18 nodejs/reliability#640
Open
32 tasks
pluris
src: add a condition if the argument of DomainToUnicode is empty
adf338b
@pluris
pluris force-pushed the fix/check_empty_input branch from 4e380fd to adf338b Compare August 18, 2023 04:18
@deokjinkim deokjinkim added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 18, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 18, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 18, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53427/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Aug 20, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53457/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
deokjinkim
deokjinkim approved these changes Aug 20, 2023
View reviewed changes
@deokjinkim deokjinkim added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. labels Aug 20, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Aug 20, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 45e5ec8 into nodejs:main Aug 20, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 20, 2023

Copy link
Copy Markdown
Collaborator

Landed in 45e5ec8
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Aug 21, 2023
CI Reliability 2023-08-21 nodejs/reliability#641
Open
CI Reliability 2023-08-22 nodejs/reliability#642
Open
CI Reliability 2023-08-23 nodejs/reliability#643
Open
@pluris pluris mentioned this pull request Aug 26, 2023
src: modify code for empty string #49336
Closed
@pluris
pluris deleted the fix/check_empty_input branch August 30, 2023 16:12
debadree25 pushed a commit that referenced this pull request Sep 1, 2023
pluris @debadree25
src: modify code for empty string
ce11e00 
PR-URL: #49336
Refs: #49097
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
Jungku Lee @UlisesGascon
src: add a condition if the argument of DomainToUnicode is empty
4a9ae31 
PR-URL: #49097
Refs: #46410
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
pluris @UlisesGascon
src: modify code for empty string
dc4de1c 
PR-URL: #49336
Refs: #49097
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
v20.7.0 proposal #49592
Merged
targos pushed a commit that referenced this pull request Nov 27, 2023
Jungku Lee @targos
src: add a condition if the argument of DomainToUnicode is empty
8c33731 
PR-URL: #49097
Refs: #46410
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
@targos targos mentioned this pull request Nov 28, 2023
v18.19.0 release proposal #50953
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
src: add a condition if the argument of DomainToUnicode is
ff49d15 
PR-URL: nodejs/node#49097
Refs: nodejs/node#46410
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
src: add a condition if the argument of DomainToUnicode is
d4dcab4 
PR-URL: nodejs/node#49097
Refs: nodejs/node#46410
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig approved these changes
@deokjinkim deokjinkim deokjinkim approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@pluris @nodejs-github-bot @anonrig @deokjinkim
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.