From 3d328f9f960ab21885d95f5eb07e6935495dcaab Mon Sep 17 00:00:00 2001 From: Phillip9587 Date: Tue, 29 Apr 2025 01:51:15 +0200 Subject: [PATCH 1/2] refactor: use stream.finished() instead of custom implementation --- index.js | 98 +++++++++++++++++++++++----------------------------- package.json | 5 +-- 2 files changed, 45 insertions(+), 58 deletions(-) diff --git a/index.js b/index.js index e68df7b..66444c7 100644 --- a/index.js +++ b/index.js @@ -20,18 +20,8 @@ module.exports.isFinished = isFinished * @private */ -var asyncHooks = tryRequireAsyncHooks() -var first = require('ee-first') - -/** - * Variables. - * @private - */ - -/* istanbul ignore next */ -var defer = typeof setImmediate === 'function' - ? setImmediate - : function (fn) { process.nextTick(fn.bind.apply(fn, arguments)) } +const asyncHooks = tryRequireAsyncHooks() +const stream = require('stream') /** * Invoke callback when the response has finished, useful for @@ -45,7 +35,7 @@ var defer = typeof setImmediate === 'function' function onFinished (msg, listener) { if (isFinished(msg) !== false) { - defer(listener, null, msg) + setImmediate(listener, null, msg) return msg } @@ -89,44 +79,65 @@ function isFinished (msg) { */ function attachFinishedListener (msg, callback) { - var eeMsg - var eeSocket - var finished = false + let finished = false + let cleanupSocket function onFinish (error) { - eeMsg.cancel() - eeSocket.cancel() - + if (finished) return finished = true callback(error) } - // finished on first message event - eeMsg = eeSocket = first([[msg, 'end', 'finish']], onFinish) + const cleanupFinished = stream.finished(msg, (error) => { + cleanupFinished() + if (cleanupSocket) { + cleanupSocket() + } + + // ignore premature close error + if (error && error.code !== 'ERR_STREAM_PREMATURE_CLOSE') { + onFinish(error) + } else { + onFinish() + } + }) function onSocket (socket) { // remove listener msg.removeListener('socket', onSocket) if (finished) return - if (eeMsg !== eeSocket) return + + function onSocketErrorOrClose (error) { + // remove listeners + socket.removeListener('error', onSocketErrorOrClose) + socket.removeListener('close', onSocketErrorOrClose) + + onFinish(error) + } // finished on first socket event - eeSocket = first([[socket, 'error', 'close']], onFinish) + socket.on('error', onSocketErrorOrClose) + socket.on('close', onSocketErrorOrClose) + + // cleanup socket listeners + cleanupSocket = function () { + socket.removeListener('error', onSocketErrorOrClose) + socket.removeListener('close', onSocketErrorOrClose) + } } if (msg.socket) { // socket already assigned onSocket(msg.socket) - return - } + } else { + // wait for socket to be assigned + msg.on('socket', onSocket) - // wait for socket to be assigned - msg.on('socket', onSocket) - - if (msg.socket === undefined) { - // istanbul ignore next: node.js 0.8 patch - patchAssignSocket(msg, onSocket) + // cleanup socket listener in case the socket is never assigned + cleanupSocket = function () { + msg.removeListener('socket', onSocket) + } } } @@ -134,12 +145,12 @@ function attachFinishedListener (msg, callback) { * Attach the listener to the message. * * @param {object} msg - * @return {function} + * @param {function} listener * @private */ function attachListener (msg, listener) { - var attached = msg.__onFinished + let attached = msg.__onFinished // create a private single listener with queue if (!attached || !attached.queue) { @@ -176,27 +187,6 @@ function createListener (msg) { return listener } -/** - * Patch ServerResponse.prototype.assignSocket for node.js 0.8. - * - * @param {ServerResponse} res - * @param {function} callback - * @private - */ - -// istanbul ignore next: node.js 0.8 patch -function patchAssignSocket (res, callback) { - var assignSocket = res.assignSocket - - if (typeof assignSocket !== 'function') return - - // res.on('socket', callback) is broken in 0.8 - res.assignSocket = function _assignSocket (socket) { - assignSocket.call(this, socket) - callback(socket) - } -} - /** * Try to require async_hooks * @private diff --git a/package.json b/package.json index 941b7c0..0b69978 100644 --- a/package.json +++ b/package.json @@ -8,9 +8,6 @@ ], "license": "MIT", "repository": "jshttp/on-finished", - "dependencies": { - "ee-first": "1.1.1" - }, "devDependencies": { "eslint": "8.17.0", "eslint-config-standard": "14.1.1", @@ -32,7 +29,7 @@ ], "scripts": { "lint": "eslint .", - "test": "mocha --reporter spec --bail --check-leaks test/", + "test": "mocha --reporter spec --check-leaks test/", "test-ci": "nyc --reporter=lcovonly --reporter=text npm test", "test-cov": "nyc --reporter=html --reporter=text npm test" } From b764cbda519551aab6338f39e0a8e5aadee72c5e Mon Sep 17 00:00:00 2001 From: Phillip9587 Date: Tue, 29 Apr 2025 02:17:01 +0200 Subject: [PATCH 2/2] Updated CI Workflow from #53 --- .github/workflows/ci.yml | 139 ++++++++++++++++++++++++++------------- 1 file changed, 92 insertions(+), 47 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3780ed3..6dbff8a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,13 +1,45 @@ name: ci on: -- pull_request -- push + pull_request: + branches: + - master + paths-ignore: + - '*.md' + push: + paths-ignore: + - '*.md' + +permissions: + contents: read + +# Cancel in progress workflows +# in the scenario where we already had a run going for that PR/branch/tag but then triggered a new run +concurrency: + group: "${{ github.workflow }} ✨ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}" + cancel-in-progress: true jobs: + lint: + name: Lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: 'lts/*' + + - name: Install dependencies + run: npm install --ignore-scripts --only=dev + + - name: Run lint + run: npm run lint + test: runs-on: ubuntu-latest strategy: + fail-fast: false matrix: name: - Node.js 0.8 @@ -34,6 +66,11 @@ jobs: - Node.js 16.x - Node.js 17.x - Node.js 18.x + - Node.js 19.x + - Node.js 20.x + - Node.js 21.x + - Node.js 22.x + - Node.js 23.x include: - name: Node.js 0.8 @@ -62,27 +99,27 @@ jobs: npm-i: mocha@3.5.3 nyc@10.3.2 - name: Node.js 4.x - node-version: "4.9" + node-version: "4" npm-i: mocha@5.2.0 nyc@11.9.0 - name: Node.js 5.x - node-version: "5.12" + node-version: "5" npm-i: mocha@5.2.0 nyc@11.9.0 - name: Node.js 6.x - node-version: "6.17" - npm-i: mocha@6.2.2 nyc@14.1.1 + node-version: "6" + npm-i: mocha@6.2.3 nyc@14.1.1 - name: Node.js 7.x - node-version: "7.10" - npm-i: mocha@6.2.2 nyc@14.1.1 + node-version: "7" + npm-i: mocha@6.2.3 nyc@14.1.1 - name: Node.js 8.0 node-version: "8.0" npm-i: mocha@6.2.2 nyc@14.1.1 - name: Node.js 8.x - node-version: "8.17" + node-version: "8" npm-i: mocha@7.2.0 nyc@14.1.1 # test fail on Node.js 9.x for unknown reasons @@ -91,38 +128,51 @@ jobs: # node-version: "9.11" # npm-i: mocha@7.2.0 nyc@14.1.1 - name: Node.js 10.x - node-version: "10.24" + node-version: "10" npm-i: mocha@8.4.0 - name: Node.js 11.x - node-version: "11.15" - npm-i: mocha@9.2.2 + node-version: "11" + npm-i: mocha@8.4.0 - name: Node.js 12.x - node-version: "12.22" - npm-i: mocha@9.2.2 + node-version: "12" - name: Node.js 13.x - node-version: "13.14" + node-version: "13" - name: Node.js 14.x - node-version: "14.19" + node-version: "14" - name: Node.js 15.x - node-version: "15.14" + node-version: "15" - name: Node.js 16.x - node-version: "16.15" + node-version: "16" - name: Node.js 17.x - node-version: "17.9" + node-version: "17" - name: Node.js 18.x - node-version: "18.1" + node-version: "18" - steps: - - uses: actions/checkout@v2 + - name: Node.js 19.x + node-version: "19" + + - name: Node.js 20.x + node-version: "20" + + - name: Node.js 21.x + node-version: "21" + + - name: Node.js 22.x + node-version: "22" + - name: Node.js 23.x + node-version: "23" + + steps: + - uses: actions/checkout@v4 - name: Install Node.js ${{ matrix.node-version }} shell: bash -eo pipefail -l {0} run: | @@ -136,7 +186,12 @@ jobs: dirname "$(nvm which ${{ matrix.node-version }})" >> "$GITHUB_PATH" - name: Configure npm - run: npm config set shrinkwrap false + run: | + if [[ "$(npm config get package-lock)" == "true" ]]; then + npm config set package-lock false + else + npm config set shrinkwrap false + fi - name: Remove npm module(s) ${{ matrix.npm-rm }} run: npm rm --silent --save-dev ${{ matrix.npm-rm }} @@ -150,8 +205,8 @@ jobs: shell: bash run: | # eslint for linting - # - remove on Node.js < 12 - if [[ "$(cut -d. -f1 <<< "${{ matrix.node-version }}")" -lt 12 ]]; then + # - remove on Node.js < 10 + if [[ "$(cut -d. -f1 <<< "${{ matrix.node-version }}")" -lt 10 ]]; then node -pe 'Object.keys(require("./package").devDependencies).join("\n")' | \ grep -E '^eslint(-|$)' | \ sort -r | \ @@ -175,53 +230,43 @@ jobs: run: | if npm -ps ls nyc | grep -q nyc; then npm run test-ci - cp coverage/lcov.info "coverage/${{ matrix.name }}.lcov" else npm test fi - - name: Lint code - if: steps.list_env.outputs.eslint != '' - run: npm run lint - - - name: Collect code coverage - if: steps.list_env.outputs.nyc != '' - run: | - if [[ -d ./coverage ]]; then - mv ./coverage "./${{ matrix.name }}" - mkdir ./coverage - mv "./${{ matrix.name }}" "./coverage/${{ matrix.name }}" - fi - - name: Upload code coverage - uses: actions/upload-artifact@v2 if: steps.list_env.outputs.nyc != '' + uses: actions/upload-artifact@v4 with: - name: coverage - path: ./coverage + name: coverage-node-${{ matrix.node-version }} + path: ./coverage/lcov.info retention-days: 1 coverage: needs: test runs-on: ubuntu-latest + permissions: + contents: read + checks: write steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v4 - name: Install lcov shell: bash run: sudo apt-get -y install lcov - name: Collect coverage reports - uses: actions/download-artifact@v2 + uses: actions/download-artifact@v4 with: - name: coverage path: ./coverage + pattern: coverage-node-* - name: Merge coverage reports shell: bash - run: find ./coverage -name lcov.info -exec printf '-a %q\n' {} \; | xargs lcov -o ./coverage/lcov.info + run: find ./coverage -name lcov.info -exec printf '-a %q\n' {} \; | xargs lcov -o ./lcov.info - name: Upload coverage report - uses: coverallsapp/github-action@master + uses: coverallsapp/github-action@v2 with: github-token: ${{ secrets.GITHUB_TOKEN }} + file: ./lcov.info \ No newline at end of file