Skip to content

Conversation

aks-
Copy link
Member

@aks- aks- commented Oct 31, 2015

No description provided.

@aks-
Copy link
Member Author

aks- commented Oct 31, 2015

cc @nodejs/http

@dougwilson
Copy link
Member

👎 , protection already exists in EventEmitter.prototype.addListener.

@aks-
Copy link
Member Author

aks- commented Oct 31, 2015

@dougwilson Oho I see. Well then should we have this check here https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L93-L97

@dougwilson
Copy link
Member

Unsure. Was added in PR #3090 , which can help us decide.

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Nov 1, 2015
@MylesBorins
Copy link
Contributor

Dupe of #3539

@aredridel
Copy link
Contributor

Kind of a dupe, anyway. That other PR covers other cases.

Choose a reason for hiding this comment

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

should be strict check IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

Preferred, yes. Although, typeof only ever returns strings.

Fishrock123 pushed a commit that referenced this pull request Nov 5, 2015
- This check is already covered in EventEmitter#addListener()

Refs: #3618
PR-URL: #3631
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@Fishrock123
Copy link
Contributor

Replacement landed in 8625a38

@Fishrock123 Fishrock123 closed this Nov 5, 2015
rvagg pushed a commit that referenced this pull request Nov 7, 2015
- This check is already covered in EventEmitter#addListener()

Refs: #3618
PR-URL: #3631
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues or PRs related to the http subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants