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

nodeav · 2018-03-27T23:13:52Z

numbers which are coerced to scientific notation via .toString(),
will behave unexpectedly when assigned to a url's port.

Fixes: #19595

Checklist

documentation is changed or added
commit message follows commit guidelines

numbers which are coerced to scientific notation via .toString(), will behave unexpectedly when assigned to a url's port. Fixes: nodejs#19595

TimothyGu · 2018-03-28T00:02:38Z

doc/api/url.md

+// 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


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

Fixed, and made the entire thing arguably clearer

Trott · 2018-03-28T05:14:53Z

doc/api/url.md

@@ -317,6 +317,15 @@ console.log(myURL.port);
 myURL.port = 1e10;
 console.log(myURL.port);
 // Prints 1234
+
+// Out-of-range numbers, which are converted to exponential 


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.

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.

Trott · 2018-03-28T05:15:37Z

Welcome @nodeav, and thanks for the pull request! Documentation changes tend to attract a lot of discussion and comments, so please be patient with us! Don't get discouraged!

TimothyGu · 2018-03-28T06:40:59Z

doc/api/url.md

+// Prints 4 (because the coefficient is 4.567)
+
+// Out-of-range numbers, which are not represented in scientific noation,
+// will be ignored.
 myURL.port = 1e10;


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.

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

vsemozhetbyt

Sorry for many nits)

vsemozhetbyt · 2018-04-02T23:01:28Z

doc/api/url.md

@@ -313,20 +313,39 @@ myURL.port = 1234.5678;
 console.log(myURL.port);
 // Prints 1234

-// Out-of-range numbers are ignored
-myURL.port = 1e10;
+// Out-of-range numbers, which are not represented in scientific noation,


Nit: noation -> notation

vsemozhetbyt · 2018-04-02T23:10:27Z

doc/api/url.md

-lies outside the range denoted above, it is ignored.
+Upon assigning a value to the port, the value will first be converted to a string using `.toString()`.
+
+If that string is invalid, but it begins with a


Not sure why, but the wrapping of these two paragraphs creates hard wraps in the rendered text: https://github.com/nodeav/node/blob/e402ff4b54dd1eeb0cd0e7ce346751e97769c880/doc/api/url.md#urlport

Maybe we can rewrap them with more long lines (but within 80 characters)?

I'm not sure I understand, in which line does the hard wrap occur?
And are we really limited to 80 characters? Existing lines in this doc are considerably longer

Yes, existing lines more than 80 chars is a temporal legacy, but now we have a rule in the STYLE_GUIDE.md and doc linting should throw on limit breaking.

I mean these hard wraps:

breaks:

Oh, I see You have trailing spaces after some added lines, and markdown parses such spaces as hard breaks. It is usually useful to trim trailing spaces in the editor before saving the doc.

Oh, the trailing spaces were actually intentional, but I don't mind changing that.

and good to know about the line widths

Trailing spaces are not usually safe to rely upon in shared sources: some collaborator may edit any other line and have their editor set for trailing spaces auto-trimming in the whole doc, so these spaces may be deleted silently in any other PR)

Is there an actual designated docs linter? How do I run it?
(I made the changes manually for now)

by the way, automatically removing trailing spaces for markdown documents is a bug IMO, as it's ignoring a part of the markdown language

I made sure the regex \ $ doesn't find any result in the document.

If I am not mistaken. this is how to lint docs locally:

make lint-md-build make test-doc

Thanks! The first one says there's nothing to be done; the second one passes.

vsemozhetbyt · 2018-04-02T23:11:20Z

doc/api/url.md

+
+Note that numbers which contain a decimal point,  
+such as floating-point numbers or numbers in scientific notation, are not an exception to this rule.  
+Leading numbers up to the decimal point will be set as the url's port, assuming they are valid. 


Nit: url's -> URL's?

vsemozhetbyt · 2018-04-02T23:12:20Z

doc/api/url.md

+```js
+myURL.port = 4.567e21;
+console.log(myURL.port);
+// Prints 4 (because it is the leading number in the string "4.567e21")


Nit: "4.567e21" -> '4.567e21'?
We usually use single quotes in code-related things.

vsemozhetbyt · 2018-04-02T23:13:25Z

doc/api/url.md

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


Nit: this empty line seems redundant.

vsemozhetbyt · 2018-04-02T23:13:42Z

doc/api/url.md

+myURL.port = 4.567e21;
+console.log(myURL.port);
+// Prints 4 (because it is the leading number in the string "4.567e21")
+


Nit: this empty line seems redundant.

vsemozhetbyt · 2018-04-03T11:44:19Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/401/

vsemozhetbyt

Doc form LGTM.
Thank you!

jasnell · 2018-04-03T14:23:45Z

doc/api/url.md

+such as floating-point numbers or numbers in scientific notation,
+are not an exception to this rule.
+Leading numbers up to the decimal point will be set as the URL's port,
+assuming they are valid.


nit: The line breaks in this are rather inconsistent.

Indeed they are. fortunately markdown doesn't care about my linebreaks :) I wanted to keep the width under 80 characters.

nodeav · 2018-04-08T07:27:01Z

@Trott @TimothyGu
I've commented on the issue (#19595) as I believe this should be fixed in the code instead; Otherwise, please let me know if you'd like any more changes to this PR.

Thanks.

Trott · 2018-04-08T15:32:43Z

doc/api/url.md

@@ -313,8 +313,9 @@ myURL.port = 1234.5678;
 console.log(myURL.port);
 // Prints 1234

-// Out-of-range numbers are ignored
-myURL.port = 1e10;
+// Out-of-range numbers, which are not represented in scientific notation,


Nit: commas should be removed here.

Trott · 2018-04-08T15:33:15Z

doc/api/url.md

+Upon assigning a value to the port, the value will first be converted to a
+string using `.toString()`.
+
+If that string is invalid, but it begins with a number, the leading number is


Nit: First comma should be removed.

Trott · 2018-04-08T15:34:05Z

doc/api/url.md

+Leading numbers up to the decimal point will be set as the URL's port,
+assuming they are valid.
+
+For example:


Nit: Remove this line and put a colon instead of a period after valid on the line of text above this one.

Trott · 2018-04-08T15:35:22Z

@Trott @TimothyGu
I've commented on the issue (#19595) as I believe this should > be fixed in the code instead; Otherwise, please let me know if you'd like any more changes to this PR.

I have some minor formatting nits, but the wording and text seems fine to me. I think this is good to land, unless code changes that make it invalid are imminent. @TimothyGu @jasnell

TimothyGu · 2018-04-08T16:40:45Z

Looks great, thanks!

nodeav · 2018-04-08T20:59:35Z

@Trott I agree with the formatting nits you have mentioned and have fixed them. Thanks for the review!

As for the code changes - for now I haven't gotten any further comments on the issue; If, however, the decision will be made to change the code, I'll be sure to change the docs as well.

BridgeAR · 2018-04-09T17:21:26Z

Lite-CI https://ci.nodejs.org/job/node-test-pull-request-lite/466/

numbers which are coerced to scientific notation via .toString(), will behave unexpectedly when assigned to a url's port. Fixes: #19595 PR-URL: #19645 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

trivikr · 2018-04-09T19:31:25Z

Landed in 51c2c51

Congrats @nodeav for your first commit in Node.js core! 🎉🎉🎉

numbers which are coerced to scientific notation via .toString(), will behave unexpectedly when assigned to a url's port. Fixes: #19595 PR-URL: #19645 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

doc: explain edge case when assigning port to url

d719151

numbers which are coerced to scientific notation via .toString(), will behave unexpectedly when assigned to a url's port. Fixes: nodejs#19595

nodejs-github-bot added doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module. labels Mar 27, 2018

TimothyGu reviewed Mar 28, 2018

View reviewed changes

Trott reviewed Mar 28, 2018

View reviewed changes

clarify some details

2cc61c1

TimothyGu reviewed Mar 28, 2018

View reviewed changes

LosBunkos added 2 commits April 3, 2018 01:09

improve docs again

257eae0

add another small note

e402ff4

vsemozhetbyt reviewed Apr 2, 2018

View reviewed changes

LosBunkos and others added 4 commits April 3, 2018 08:21

fix typo

64863ee

remove redundant newlines and small styling fix

aedadce

remove trailing spaces and limit line length to 80

a55b432

remove more trailing spaces

0d39ca6

vsemozhetbyt approved these changes Apr 3, 2018

View reviewed changes

jasnell reviewed Apr 3, 2018

View reviewed changes

jasnell approved these changes Apr 3, 2018

View reviewed changes

Trott reviewed Apr 8, 2018

View reviewed changes

fix some nits

92919bc

TimothyGu approved these changes Apr 8, 2018

View reviewed changes

BridgeAR approved these changes Apr 9, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 9, 2018

trivikr closed this Apr 9, 2018

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

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

Conversation

nodeav commented Mar 27, 2018

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

vsemozhetbyt Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt commented Apr 3, 2018

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodeav commented Apr 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Apr 8, 2018

TimothyGu commented Apr 8, 2018

nodeav commented Apr 8, 2018

BridgeAR commented Apr 9, 2018

trivikr commented Apr 9, 2018

vsemozhetbyt Apr 3, 2018 •

edited

Loading

vsemozhetbyt Apr 3, 2018 •

edited

Loading

vsemozhetbyt Apr 3, 2018 •

edited

Loading