Pull latest changes from nodejs/node #28
Conversation
|
@aduh95 any change you open this pr from a branch on this repo so I can check it out and tackle with it? |
That's not exact, Node.js v14.x has
Any reason why you can't do from my fork? As a maintainer of this repo, you even should be allowed to push commit on my branch if needed. (The easiest to checkout a PR is to run |
aduh95
force-pushed
the
pull-latest-commits-from-node-core
branch
3 times, most recently
from
fae94be to
86679ee
Compare
MoLow
force-pushed
the
pull-latest-commits-from-node-core
branch
2 times, most recently
from
237798a to
1468888
Compare
aduh95
force-pushed
the
pull-latest-commits-from-node-core
branch
from
1468888 to
c06ee1c
Compare
PR-URL: nodejs/node#43554 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> (cherry picked from commit 389b7e138e89a339fabe4ad628bf09cd9748f957)
aduh95
force-pushed
the
pull-latest-commits-from-node-core
branch
2 times, most recently
from
93fbd06 to
41c0c3a
Compare
PR-URL: nodejs/node#43919 Reviewed-By: Mestery <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> (cherry picked from commit 2fd4c013c221653da2a7921d08fe1aa96aaba504)
aduh95
force-pushed
the
pull-latest-commits-from-node-core
branch
from
41c0c3a to
10146fa
Compare
PR-URL: nodejs/node#43911 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> (cherry picked from commit 2e682f10b6104373fded64b0e364984b85ae2243)
PR-URL: nodejs/node#43843 Reviewed-By: Feng Yu <[email protected]> (cherry picked from commit d83446b4c4694322e12d2b7d22592f2be674e580)
PR-URL: nodejs/node#43887 Fixes: nodejs/node#43837 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Jacob Smith <[email protected]> (cherry picked from commit dab492f0444b0a6ae8a41dd1d9605e036c363655)
aduh95
force-pushed
the
pull-latest-commits-from-node-core
branch
from
ed3cd03 to
ba8fd71
Compare
| @@ -102,6 +102,43 @@ function expectsError (validator, exact) { | |||
| }, exact) | |||
| } | |||
|
|
|||
| if (typeof AbortSignal.timeout !== 'function') { | |||
There was a problem hiding this comment.
will this be required in consumer code too? Would it be safer to use a AbortSignal ponyfill?
There was a problem hiding this comment.
no, it is not needed, these adjustments are just so the tap output will match across node versions
There was a problem hiding this comment.
Great, thank you!
| if (typeof AbortSignal.timeout !== 'function') { | |
| // Ensure test output is consistent across node versions | |
| if (typeof AbortSignal.timeout !== 'function') { |
There was a problem hiding this comment.
AbortSignal.timeout doesn't exist on Node.js 14.x, so all the tests that use it fails on that version. It's not required though, test/node-core-test.js makes sure that the library still works when it's not polyfilled.
The section below, that overrides AbortSignal.abort and AbortController.prototype.abort is indeed there just for the TAP output to match, and same, we have test/node-core-test.js to ensure it's not necessary for the lib to work.
There was a problem hiding this comment.
I'm going to land this and open a follow-up PR that adds the comments.
Tests are failing on v16.x and v14.x, not sure why.