Conversation
* fix Request.prototype.closed() clash with new Reader.closed (GH-1888) * fix IPv6 url by supporting new server.address().family format * add node 18 to GH Actions
|
@DonutEspresso thoughts? |
|
Is there is a roadmap for reviewing and merging node18 support? How can I help? |
|
Are there any updates on this PR? We are blocked from a security update until this is solved. |
|
Can you advise what is the preferred path to add support to Node 18? |
|
FWIW here's another ping. We'd love to be on node 18, but can't start up without this patch. |
| * @see https://github.com/restify/node-restify/issues/1888 | ||
| */ | ||
| Object.defineProperty(Request.prototype, 'closed', { | ||
| get: function getter(x) { |
There was a problem hiding this comment.
If this code continues to remain necessary (see my overall review comment) @ghermeto Please remove the x. @meteorlxy is correct that it is unnecessary.
sternam
left a comment
sternam
left a comment
There was a problem hiding this comment.
One minor comment if we stay with this approach, but at a higher level, why isn't it preferable to address the problem by refactoring restify to just use the closed getter directly instead of relying on Request having a closed property? Then this change would be unnecessary.
| * @see https://github.com/restify/node-restify/issues/1888 | ||
| */ | ||
| Object.defineProperty(Request.prototype, 'closed', { | ||
| get: function getter(x) { |
There was a problem hiding this comment.
If this code continues to remain necessary (see my overall review comment) @ghermeto Please remove the x. @meteorlxy is correct that it is unnecessary.
| var self = this; | ||
| return self.connectionState() === 'close'; | ||
| }; | ||
| if (this._readableState && 'close' in this._readableState) { |
There was a problem hiding this comment.
typo
| if (this._readableState && 'close' in this._readableState) { | |
| if (this._readableState && 'closed' in this._readableState) { |
and tests are failing with typo fixed. I'm trying to figure out if there's a way to do this without monkey patching.
|
One alternative we're considering is to just remove the |
|
Closing since we're moving forward with #1929 |
7 participants
Request.prototype.closedpatch clashes with new Readable.closed getter #1888)What?
Backward compatible change to support Node v18.
Why?
Node v18 breaking changes:
Reader.closedgetter that clashes with theRequest.prototype.closed()patch.server.address().familyfromstring(IPv6) to anumber(6)Drawbacks
I don't particularly like this change because it will be unexpected to people using Node18 that
closedwill be a method and not a getter. I would prefer to limit Restify 8.x to Node 17 and make a breaking change for 9.x by changing the method name toisClosed()or better yet, drop the method in favor of patching a getter for node v10~v17.@restify/current-core please advise on the preferred action plan.