http: fix legacy http.Client instanceof #8103
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
Ensures that objects created with http.createClient() have http.Client.prototype as their prototype.
|
Good catch. Maybe this change should just have assigned the prototype instead of extending the chain, i.e. |
|
We should probably remove this altogether in v7. The old http client API has been (hard) deprecated at least since 2012. |
|
@mscdex Yup, for sure. For now though I think this needs to be unbroken for LTS branches. @addaleax Hmm that's probably a more correct change, and fixes any general case of deprecating both the class and the factory like was done here. Also has the bonus of not touching deprecated code. I'll give that a shot, and if it works and breaks nothing, I'll PR-ify it and close this one. |
bengl
added a commit
to bengl/node
that referenced
this pull request
Aug 14, 2016
Ensure the wrapped class prototype is exactly the unwrapped class prototype, rather than an object whose prototype is the unwrapped class prototype. This ensures that instances of the unwrapped class are instances of the wrapped class. This is useful when both a wrapped class and a factory for the unwrapped class are both exposed. Ref: nodejs#8103
4 tasks
|
Closing in favor of #8105 |
jasnell
pushed a commit
that referenced
this pull request
Aug 18, 2016
Ensure the wrapped class prototype is exactly the unwrapped class prototype, rather than an object whose prototype is the unwrapped class prototype. This ensures that instances of the unwrapped class are instances of the wrapped class. This is useful when both a wrapped class and a factory for the unwrapped class are both exposed. Ref: #8103 PR-URL: #8105 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas
pushed a commit
that referenced
this pull request
Aug 20, 2016
Ensure the wrapped class prototype is exactly the unwrapped class prototype, rather than an object whose prototype is the unwrapped class prototype. This ensures that instances of the unwrapped class are instances of the wrapped class. This is useful when both a wrapped class and a factory for the unwrapped class are both exposed. Ref: #8103 PR-URL: #8105 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>
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.
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
http
Description of change
Ensures that objects created with
http.createClient()havehttp.Client.prototypeas their prototype.The new
internalUtil.deprecate()fixes instantiation of deprecated classes, but in this case, since the unwrapped deprecated class is instantiated and returned, the prototype on the created object is not the same as the exposed class.Setting the prototype on the new object to be prototype of the wrapped/exposed class fixes this, so now the following works fine:
Alternatives
Object.create(http.Client.prototype)and then call the unwrapped constructor on the result (e.g. withClient.call(client, port, host)). I think this is pretty much equivalent to what I did, so I'm happy to change it to this if people think it's cleaner.instanceofrelationship) that was broken by deprecating.