From 2f6745019ac25785032ac3659c2433b6e224f383 Mon Sep 17 00:00:00 2001 From: Erika <3019731+Princesseuh@users.noreply.github.com> Date: Fri, 6 Jan 2023 18:01:54 +0100 Subject: [PATCH] Drop Node 14 in CI for Node 16 and add Node 18 to the matrix (#5768) * ci(node): Move CI to Node 16 and add Node 18 to the matrix * fix(netlify): Fix set-cookie not working on Node 18 * fix(netlify): Handle if `set-cookie` is already somehow an array (apparently it can?) * test(node): Fix `toPromise` to match Astro's * fix(tests): Use the actual underlying ArrayBuffer instance to create the buffer in toPromise * chore: changeset --- .changeset/modern-bulldogs-film.md | 5 + .github/workflows/benchmark.yml | 18 +-- .github/workflows/ci.yml | 11 +- packages/astro/test/units/test-utils.js | 6 +- .../netlify/src/netlify-functions.ts | 117 ++++++++++++++++-- packages/integrations/node/test/test-utils.js | 13 +- 6 files changed, 138 insertions(+), 32 deletions(-) create mode 100644 .changeset/modern-bulldogs-film.md diff --git a/.changeset/modern-bulldogs-film.md b/.changeset/modern-bulldogs-film.md new file mode 100644 index 000000000..17b0b68be --- /dev/null +++ b/.changeset/modern-bulldogs-film.md @@ -0,0 +1,5 @@ +--- +'@astrojs/netlify': patch +--- + +Fix set-cookies not working in certain cases when using Node 18+ diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 0fd11435c..ccc6d3660 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -16,13 +16,13 @@ jobs: permissions: contents: read outputs: - PR-BENCH-14: ${{ steps.benchmark-pr.outputs.BENCH_RESULT14 }} PR-BENCH-16: ${{ steps.benchmark-pr.outputs.BENCH_RESULT16 }} - MAIN-BENCH-14: ${{ steps.benchmark-main.outputs.BENCH_RESULT14 }} + PR-BENCH-18: ${{ steps.benchmark-pr.outputs.BENCH_RESULT18 }} MAIN-BENCH-16: ${{ steps.benchmark-main.outputs.BENCH_RESULT16 }} + MAIN-BENCH-18: ${{ steps.benchmark-main.outputs.BENCH_RESULT18 }} strategy: matrix: - node-version: [14, 16] + node-version: [16, 18] steps: - uses: actions/checkout@v3 with: @@ -89,12 +89,12 @@ jobs: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} pr_number: ${{ github.event.issue.number }} message: | - **Node**: 14 - **PR**: ${{ needs.benchmark.outputs.PR-BENCH-14 }} - **MAIN**: ${{ needs.benchmark.outputs.MAIN-BENCH-14 }} - - --- - **Node**: 16 **PR**: ${{ needs.benchmark.outputs.PR-BENCH-16 }} **MAIN**: ${{ needs.benchmark.outputs.MAIN-BENCH-16 }} + + --- + + **Node**: 18 + **PR**: ${{ needs.benchmark.outputs.PR-BENCH-18 }} + **MAIN**: ${{ needs.benchmark.outputs.MAIN-BENCH-18 }} diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 86cf1cd5a..0834bd014 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -82,7 +82,7 @@ jobs: strategy: matrix: OS: [ubuntu-latest] - NODE_VERSION: [14] + NODE_VERSION: [16] fail-fast: true steps: - name: Checkout @@ -111,11 +111,10 @@ jobs: strategy: matrix: OS: [ubuntu-latest, windows-latest] - # TODO: Enable node@18! - NODE_VERSION: [14, 16] + NODE_VERSION: [16, 18] include: - os: macos-latest - NODE_VERSION: 14 + NODE_VERSION: 16 fail-fast: false env: NODE_VERSION: ${{ matrix.NODE_VERSION }} @@ -154,7 +153,7 @@ jobs: strategy: matrix: OS: [ubuntu-latest, windows-latest] - NODE_VERSION: [14] + NODE_VERSION: [16] fail-fast: false env: NODE_VERSION: ${{ matrix.NODE_VERSION }} @@ -188,7 +187,7 @@ jobs: strategy: matrix: OS: [ubuntu-latest, windows-latest] - NODE_VERSION: [14] + NODE_VERSION: [16] env: NODE_VERSION: ${{ matrix.NODE_VERSION }} steps: diff --git a/packages/astro/test/units/test-utils.js b/packages/astro/test/units/test-utils.js index 38c25d16c..f142fceaf 100644 --- a/packages/astro/test/units/test-utils.js +++ b/packages/astro/test/units/test-utils.js @@ -1,9 +1,9 @@ -import httpMocks from 'node-mocks-http'; import { EventEmitter } from 'events'; import { Volume } from 'memfs'; +import httpMocks from 'node-mocks-http'; import realFS from 'node:fs'; -import { fileURLToPath } from 'url'; import npath from 'path'; +import { fileURLToPath } from 'url'; import { unixify } from './correct-path.js'; class VirtualVolume extends Volume { @@ -125,7 +125,7 @@ export function toPromise(res) { const write = res.write; res.write = function (data, encoding) { if (ArrayBuffer.isView(data) && !Buffer.isBuffer(data)) { - data = Buffer.from(data); + data = Buffer.from(data.buffer); } return write.call(this, data, encoding); }; diff --git a/packages/integrations/netlify/src/netlify-functions.ts b/packages/integrations/netlify/src/netlify-functions.ts index fe6ce7a15..57b042af9 100644 --- a/packages/integrations/netlify/src/netlify-functions.ts +++ b/packages/integrations/netlify/src/netlify-functions.ts @@ -104,19 +104,29 @@ export const createExports = (manifest: SSRManifest, args: Args) => { // The fetch API does not have a way to get multiples of a single header, but instead concatenates // them. There are non-standard ways to do it, and node-fetch gives us headers.raw() // See https://github.com/whatwg/fetch/issues/973 for discussion - if (response.headers.has('set-cookie') && 'raw' in response.headers) { - // Node fetch allows you to get the raw headers, which includes multiples of the same type. - // This is needed because Set-Cookie *must* be called for each cookie, and can't be - // concatenated together. - type HeadersWithRaw = Headers & { - raw: () => Record; - }; - - const rawPacked = (response.headers as HeadersWithRaw).raw(); - if ('set-cookie' in rawPacked) { - fnResponse.multiValueHeaders = { - 'set-cookie': rawPacked['set-cookie'], + if (response.headers.has('set-cookie')) { + if ('raw' in response.headers) { + // Node fetch allows you to get the raw headers, which includes multiples of the same type. + // This is needed because Set-Cookie *must* be called for each cookie, and can't be + // concatenated together. + type HeadersWithRaw = Headers & { + raw: () => Record; }; + + const rawPacked = (response.headers as HeadersWithRaw).raw(); + if ('set-cookie' in rawPacked) { + fnResponse.multiValueHeaders = { + 'set-cookie': rawPacked['set-cookie'], + }; + } + } else { + const cookies = response.headers.get('set-cookie'); + + if (cookies) { + fnResponse.multiValueHeaders = { + 'set-cookie': Array.isArray(cookies) ? cookies : splitCookiesString(cookies), + }; + } } } @@ -135,3 +145,86 @@ export const createExports = (manifest: SSRManifest, args: Args) => { return { handler }; }; + +/* + From: https://github.com/nfriedly/set-cookie-parser/blob/5cae030d8ef0f80eec58459e3583d43a07b984cb/lib/set-cookie.js#L144 + Set-Cookie header field-values are sometimes comma joined in one string. This splits them without choking on commas + that are within a single set-cookie field-value, such as in the Expires portion. + This is uncommon, but explicitly allowed - see https://tools.ietf.org/html/rfc2616#section-4.2 + Node.js does this for every header *except* set-cookie - see https://github.com/nodejs/node/blob/d5e363b77ebaf1caf67cd7528224b651c86815c1/lib/_http_incoming.js#L128 + React Native's fetch does this for *every* header, including set-cookie. + Based on: https://github.com/google/j2objc/commit/16820fdbc8f76ca0c33472810ce0cb03d20efe25 + Credits to: https://github.com/tomball for original and https://github.com/chrusart for JavaScript implementation +*/ +function splitCookiesString(cookiesString: string): string[] { + if (Array.isArray(cookiesString)) { + return cookiesString; + } + if (typeof cookiesString !== 'string') { + return []; + } + + let cookiesStrings = []; + let pos = 0; + let start; + let ch; + let lastComma; + let nextStart; + let cookiesSeparatorFound; + + function skipWhitespace() { + while (pos < cookiesString.length && /\s/.test(cookiesString.charAt(pos))) { + pos += 1; + } + return pos < cookiesString.length; + } + + function notSpecialChar() { + ch = cookiesString.charAt(pos); + + return ch !== '=' && ch !== ';' && ch !== ','; + } + + while (pos < cookiesString.length) { + start = pos; + cookiesSeparatorFound = false; + + while (skipWhitespace()) { + ch = cookiesString.charAt(pos); + if (ch === ',') { + // ',' is a cookie separator if we have later first '=', not ';' or ',' + lastComma = pos; + pos += 1; + + skipWhitespace(); + nextStart = pos; + + while (pos < cookiesString.length && notSpecialChar()) { + pos += 1; + } + + // currently special character + if (pos < cookiesString.length && cookiesString.charAt(pos) === '=') { + // we found cookies separator + cookiesSeparatorFound = true; + // pos is inside the next cookie, so back up and return it. + pos = nextStart; + cookiesStrings.push(cookiesString.substring(start, lastComma)); + start = pos; + } else { + // in param ',' or param separator ';', + // we continue from that comma + pos = lastComma + 1; + } + } else { + pos += 1; + } + } + + if (!cookiesSeparatorFound || pos >= cookiesString.length) { + cookiesStrings.push(cookiesString.substring(start, cookiesString.length)); + } + } + + return cookiesStrings; +} diff --git a/packages/integrations/node/test/test-utils.js b/packages/integrations/node/test/test-utils.js index 0859c6acd..d3d7c17be 100644 --- a/packages/integrations/node/test/test-utils.js +++ b/packages/integrations/node/test/test-utils.js @@ -1,6 +1,6 @@ -import { loadFixture as baseLoadFixture } from '../../../astro/test/test-utils.js'; -import httpMocks from 'node-mocks-http'; import { EventEmitter } from 'events'; +import httpMocks from 'node-mocks-http'; +import { loadFixture as baseLoadFixture } from '../../../astro/test/test-utils.js'; /** * @typedef {import('../../../astro/test/test-utils').Fixture} Fixture @@ -33,6 +33,15 @@ export function createRequestAndResponse(reqOptions) { export function toPromise(res) { return new Promise((resolve) => { + // node-mocks-http doesn't correctly handle non-Buffer typed arrays, + // so override the write method to fix it. + const write = res.write; + res.write = function (data, encoding) { + if (ArrayBuffer.isView(data) && !Buffer.isBuffer(data)) { + data = Buffer.from(data.buffer); + } + return write.call(this, data, encoding); + }; res.on('end', () => { let chunks = res._getChunks(); resolve(chunks);