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

Cherrypick commits from joyent/node #834

Closed
wants to merge 7 commits into from
Closed

Cherrypick commits from joyent/node #834

wants to merge 7 commits into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Feb 13, 2015

These are the commits listed in #832 minus nodejs/node-v0.x-archive@ad06848, which I'll handle in #278.

Closes #832
R=@bnoordhuis

cjihrig and others added 7 commits February 13, 2015 12:02
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
@bnoordhuis
Copy link
Member

@cjihrig
Copy link
Contributor Author

cjihrig commented Feb 13, 2015

The only commit here that has any semver implications is faafbe7 (throw on invalid socket timeout). How do we want to label it?

@bnoordhuis
Copy link
Member

Semver-minor? You could argue it's a bug fix but it's probably the path of least resistance to bump the minor.

@mikeal
Copy link
Contributor

mikeal commented Feb 13, 2015

Proper argument checking isn't a breaking change if the invalid input broke silently in prior releases.

@cjihrig
Copy link
Contributor Author

cjihrig commented Feb 13, 2015

@mikeal it definitely lead to confusion. See nodejs/node-v0.x-archive#8618

@cjihrig
Copy link
Contributor Author

cjihrig commented Feb 13, 2015

OK, the CI looks happy. Is anyone opposed to this landing right away?

@mikeal
Copy link
Contributor

mikeal commented Feb 13, 2015

@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 :)

@Fishrock123
Copy link
Contributor

Looks like a patch to me.

@cjihrig
Copy link
Contributor Author

cjihrig commented Feb 13, 2015

Thanks for the feedback. Landed in 0cff052, 8c1df7a, 6a2b204, 30dca66, 3b1b4de, 4ca7cca, and cca8de6

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.

7 participants