Skip to content

doc: explain edge case when assigning port to url #19645

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

Closed
nodeav wants to merge 9 commits into from
Closed

doc: explain edge case when assigning port to url #19645

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d719151
doc: explain edge case when assigning port to url
nodeav Mar 27, 2018
2cc61c1
clarify some details
LosBunkos Mar 28, 2018
257eae0
improve docs again
LosBunkos Apr 2, 2018
e402ff4
add another small note
LosBunkos Apr 2, 2018
64863ee
fix typo
LosBunkos Apr 3, 2018
aedadce
remove redundant newlines and small styling fix
LosBunkos Apr 3, 2018
a55b432
remove trailing spaces and limit line length to 80
Apr 3, 2018
0d39ca6
remove more trailing spaces
Apr 3, 2018
92919bc
fix some nits
Apr 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions doc/api/url.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,15 @@ console.log(myURL.port);
myURL.port = 1e10;
Copy link
Copy Markdown
Member

@TimothyGu TimothyGu Mar 28, 2018

Choose a reason for hiding this comment

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

I guess it isn't super clear that (1e10).toString() is '10000000000' -- might be good to clarify that.

It got me thinking that it might be good to just describe the actual algorithm for assigning a number to port in the prose (ll. 322-329): the number is first stringified using String(num), and then the leading characters of the resulting string that are digits are then converted back to a number. (Note: in cases where the default representation of the number is an exponential, the parts after the decimal point or the e character are discarded.) Only then is the range checked.

Copy link
Copy Markdown
Contributor Author

@nodeav nodeav Apr 2, 2018

Choose a reason for hiding this comment

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

I've rephrased the whole thing, adopting most of your advice, and it is much clearer now in my opinion.

console.log(myURL.port);
// Prints 1234

Copy link
Copy Markdown
Contributor

@vsemozhetbyt vsemozhetbyt Apr 2, 2018

Choose a reason for hiding this comment

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

Nit: this empty line seems redundant.

// Out-of-range numbers, which are converted to exponential
Copy link
Copy Markdown
Member

@Trott Trott Mar 28, 2018

Choose a reason for hiding this comment

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

This is especially confusing given the previous example which says unconditionally that out-of-range numbers are ignored. Are we sure this behavior isn't simply a bug that needs fixing, rather than a defined behavior that needs documenting? It's hard to imagine anyone actually intentionally using this behavior.

Copy link
Copy Markdown
Contributor Author

@nodeav nodeav Mar 28, 2018

Choose a reason for hiding this comment

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

I agree with both points. I didn't have time to go through the actual spec, however I was told it is indeed the case in:
#19595
whatwg/url#377

Obviously adding a range-check and some tests is a very straightforward solution for the unexpected behavior, but I'm not at all sure this is not a spec bug.

In the meantime though, the documentation should perhaps be changed as it does not reflect the actual behavior expressed in node.

// notation via .toString(), will behave as strings with leading zeroes.
// This means that the port will be assigned the integer part of the coefficient,
// assuming the number is normalized (for example, 0.9e10 => 9e9).
// See https://www.ecma-international.org/ecma-262/6.0/#sec-tostring-applied-to-the-number-type for more information
Copy link
Copy Markdown
Member

@TimothyGu TimothyGu Mar 28, 2018

Choose a reason for hiding this comment

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

I'd omit the spec link. I also think "will behave as strings with leading zeroes" might be more confusing than clarifying.

Copy link
Copy Markdown
Contributor Author

@nodeav nodeav Mar 28, 2018

Choose a reason for hiding this comment

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

Fixed, and made the entire thing arguably clearer

myURL.port = 4.567e21;
console.log(myURL.port);
// Prints 4 (because the coefficient is 4.567)
```

The port value may be set as either a number or as a String containing a number
Expand Down