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

@@ -0,0 +1,5 @@
+---
+'@astrojs/netlify': patch
+---
+
+Fix set-cookies not working in certain cases when using Node 18+

.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 }}

.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:

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);
 		};

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<string, string[]>;
-			};
-
-			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<string, string[]>;
 				};
+
+				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;
+}

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);