Cherrypick commits from joyent/node #834
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit restricts socket timeouts non-negative, finite numbers. Any other value throws a TypeError or RangeError. This prevents subtle bugs that can happen due to type coercion. Fixes: nodejs/node-v0.x-archive#8618 PR-URL: nodejs/node-v0.x-archive#8884 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com> Conflicts: lib/timers.js test/simple/test-net-settimeout.js test/simple/test-net-socket-timeout.js
The message argument is optional for both assert() and assert.ok(). This commit makes message optional for assert(). PR-URL: nodejs/node-v0.x-archive#9003 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
The NativeModule system passes NativeModule.require transparently and so is unnecessary to call explicitly. The only one which should have the prefix is the in line 295, where actually implements a big fs-based module system and actually requires a native module. That is left unchanged. PR-URL: nodejs/node-v0.x-archive#9201 Ref: nodejs/node-v0.x-archive#2009 Reviewed-by: Trevor Norris <trev.norris@gmail.com> Conflicts: lib/module.js
Add a ';' to the end of a function call in documentation. PR-URL: nodejs/node-v0.x-archive#9198 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Timeout#unref() call returns undefined, not this. The test already worked before, because the interval was still unref'd, and the test also succeeds without clearing the interval. PR-URL: nodejs/node-v0.x-archive#9171 Reviewed-by: Colin Ihrig <cjihrig@gmail.com> Reviewed-by: Timothy J Fontaine <tjfontaine@gmail.com> Conflicts: test/simple/test-timers-unref.js
PR-URL: nodejs/node-v0.x-archive#9164 Reviewed-by: Colin Ihrig <cjihrig@gmail.com>
The current implementation uses the arguments object in the Server() constructor. Since both arguments to Server() are optional, there was a high likelihood of accessing a non-existent element in arguments, which carries a performance overhead. This commit replaces the arguments object with named arguments. Reviewed-by: Trevor Norris <trev.norris@gmail.com> Conflicts: lib/net.js
|
The only commit here that has any semver implications is faafbe7 (throw on invalid socket timeout). How do we want to label it? |
|
Semver-minor? You could argue it's a bug fix but it's probably the path of least resistance to bump the minor. |
|
Proper argument checking isn't a breaking change if the invalid input broke silently in prior releases. |
|
@mikeal it definitely lead to confusion. See nodejs/node-v0.x-archive#8618 |
|
OK, the CI looks happy. Is anyone opposed to this landing right away? |
|
@cjihrig sure, but if it's just confusion up front vs. the confusion people used to have way down the line in production I have a hard time considering it a break. Let me put it this way, the current API does not support arguments of a certain type, and likely never has. It used to fail silently which was a bug which we have now fixed :) |
|
Looks like a patch to me. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
These are the commits listed in #832 minus nodejs/node-v0.x-archive@ad06848, which I'll handle in #278.
Closes #832
R=@bnoordhuis