[cli] Fix dev query string merging (vercel#8183)

Since `vercel dev` performs query string merging with `vercel.json` routes and the requested url, there is sometimes a slight difference between the request that the browser sends and the request that the upstream dev server receives.

Most servers treat the query string key with "empty" value the same as undefined value. Meaning, these two URLs should be equivalent because each has empty values:

- `http://example.com/src/App.vue?vue&type=style&index=0&lang.css`
- `http://example.com/src/App.vue?vue=&type=style&index=0&lang.css=`

However, `vite dev` handles these two URLs differently because of [string comparisons](https://github.com/vitejs/vite/blob/2289d04af5398791c3a01c9e597dc976e593c852/packages/plugin-vue/src/handleHotUpdate.ts#L98-L99) instead of URL parsing, which causes requests from `vercel dev` to fail. Thus, this PR changes from Node.js native URL parsing to a custom implementation in order to handle this corner case and preserve the difference between `?a=` and `?a`.

Ideally this would be [fixed in vite](vitejs/vite#9589), however we might run into other dev servers that also fail in the same way in the future.

- Fixes vercel#7283
- Closes vitejs/vite#9589
- Related to nodejs/node#9500

Author: styfle

packages/cli/src/util/dev/parse-query-string.ts

@@ -0,0 +1,59 @@
+/**
+ * This function is necessary to account for the difference between
+ * `?a=` and `?a` because native `url.parse(str, true)` can't tell.
+ * @param querystring - The querystring to parse, also known as the "search" string.
+ */
+export function parseQueryString(
+  querystring?: string
+): Record<string, string[]> {
+  const query: Record<string, string[]> = Object.create(null);
+  if (!querystring || !querystring.startsWith('?') || querystring === '?') {
+    return query;
+  }
+  const params = querystring.slice(1).split('&');
+  for (let param of params) {
+    let [key, value] = param.split('=');
+    if (key !== undefined) {
+      key = decodeURIComponent(key);
+    }
+    if (value !== undefined) {
+      value = decodeURIComponent(value);
+    }
+
+    let existing = query[key];
+    if (!existing) {
+      existing = [];
+      query[key] = existing;
+    }
+
+    existing.push(value);
+  }
+  return query;
+}
+
+/**
+ * This function is necessary to account for the difference between
+ * `?a=` and `?a` because native `url.format({ query })` can't tell.
+ * @param query - The query object to stringify.
+ */
+export function formatQueryString(
+  query: Record<string, string[]> | undefined
+): string | undefined {
+  if (!query) {
+    return undefined;
+  }
+  let s = '';
+  let prefix = '?';
+  for (let [key, values] of Object.entries(query)) {
+    for (let value of values) {
+      s += prefix;
+      s += encodeURIComponent(key);
+      if (value !== undefined) {
+        s += '=';
+        s += encodeURIComponent(value);
+      }
+      prefix = '&';
+    }
+  }
+  return s || undefined;
+}

packages/cli/src/util/dev/router.ts

@@ -6,6 +6,7 @@ import DevServer from './server';
 
 import { VercelConfig, HttpHeadersConfig, RouteResult } from './types';
 import { isHandler, Route, HandleValue } from '@vercel/routing-utils';
+import { parseQueryString } from './parse-query-string';
 
 export function resolveRouteParameters(
   str: string,
@@ -56,7 +57,8 @@ export async function devRouter(
   phase?: HandleValue | null
 ): Promise<RouteResult> {
   let result: RouteResult | undefined;
-  let { query, pathname: reqPathname = '/' } = url.parse(reqUrl, true);
+  let { pathname: reqPathname = '/', search: reqSearch } = url.parse(reqUrl);
+  const reqQuery = parseQueryString(reqSearch);
   const combinedHeaders: HttpHeadersConfig = { ...previousHeaders };
   let status: number | undefined;
   let isContinue = false;
@@ -174,7 +176,7 @@ export async function devRouter(
             isDestUrl,
             status: routeConfig.status || status,
             headers: combinedHeaders,
-            uri_args: query,
+            query: reqQuery,
             matched_route: routeConfig,
             matched_route_idx: idx,
             phase,
@@ -184,17 +186,19 @@ export async function devRouter(
           if (!destPath.startsWith('/')) {
             destPath = `/${destPath}`;
           }
-          const destParsed = url.parse(destPath, true);
-          Object.assign(destParsed.query, query);
+          const { pathname: destPathname = '/', search: destSearch } =
+            url.parse(destPath);
+          const destQuery = parseQueryString(destSearch);
+          Object.assign(destQuery, reqQuery);
           result = {
             found: true,
-            dest: destParsed.pathname || '/',
+            dest: destPathname,
             continue: isContinue,
             userDest: Boolean(routeConfig.dest),
             isDestUrl,
             status: routeConfig.status || status,
             headers: combinedHeaders,
-            uri_args: destParsed.query,
+            query: destQuery,
             matched_route: routeConfig,
             matched_route_idx: idx,
             phase,
@@ -212,7 +216,7 @@ export async function devRouter(
       continue: isContinue,
       status,
       isDestUrl: false,
-      uri_args: query,
+      query: reqQuery,
       headers: combinedHeaders,
       phase,
     };

packages/cli/src/util/dev/server.ts

@@ -94,6 +94,7 @@ import { ProjectEnvVariable, ProjectSettings } from '../../types';
 import exposeSystemEnvs from './expose-system-envs';
 import { treeKill } from '../tree-kill';
 import { nodeHeadersToFetchHeaders } from './headers';
+import { formatQueryString, parseQueryString } from './parse-query-string';
 import {
   errorToString,
   isErrnoException,
@@ -1396,9 +1397,11 @@ export default class DevServer {
 
     const getReqUrl = (rr: RouteResult): string | undefined => {
       if (rr.dest) {
-        if (rr.uri_args) {
-          const destParsed = url.parse(rr.dest, true);
-          Object.assign(destParsed.query, rr.uri_args);
+        if (rr.query) {
+          const destParsed = url.parse(rr.dest);
+          const destQuery = parseQueryString(destParsed.search);
+          Object.assign(destQuery, rr.query);
+          destParsed.search = formatQueryString(destQuery);
           return url.format(destParsed);
         }
         return rr.dest;
@@ -1533,9 +1536,8 @@ export default class DevServer {
 
             // Retain orginal pathname, but override query parameters from the rewrite
             const beforeRewriteUrl = req.url || '/';
-            const rewriteUrlParsed = url.parse(beforeRewriteUrl, true);
-            delete rewriteUrlParsed.search;
-            rewriteUrlParsed.query = url.parse(rewritePath, true).query;
+            const rewriteUrlParsed = url.parse(beforeRewriteUrl);
+            rewriteUrlParsed.search = url.parse(rewritePath).search;
             req.url = url.format(rewriteUrlParsed);
             debug(
               `Rewrote incoming HTTP URL from "${beforeRewriteUrl}" to "${req.url}"`
@@ -1594,9 +1596,10 @@ export default class DevServer {
 
       if (routeResult.isDestUrl) {
         // Mix the `routes` result dest query params into the req path
-        const destParsed = url.parse(routeResult.dest, true);
-        delete destParsed.search;
-        Object.assign(destParsed.query, routeResult.uri_args);
+        const destParsed = url.parse(routeResult.dest);
+        const destQuery = parseQueryString(destParsed.search);
+        Object.assign(destQuery, routeResult.query);
+        destParsed.search = formatQueryString(destQuery);
         const destUrl = url.format(destParsed);
 
         debug(`ProxyPass: ${destUrl}`);
@@ -1737,7 +1740,7 @@ export default class DevServer {
       throw new Error('Expected Route Result but none was found.');
     }
 
-    const { dest, headers, uri_args } = routeResult;
+    const { dest, query, headers } = routeResult;
 
     // Set any headers defined in the matched `route` config
     for (const [name, value] of Object.entries(headers)) {
@@ -1773,10 +1776,11 @@ export default class DevServer {
         }
 
         this.setResponseHeaders(res, requestId);
-        const origUrl = url.parse(req.url || '/', true);
-        delete origUrl.search;
+        const origUrl = url.parse(req.url || '/');
+        const origQuery = parseQueryString(origUrl.search);
         origUrl.pathname = dest;
-        Object.assign(origUrl.query, uri_args);
+        Object.assign(origQuery, query);
+        origUrl.search = formatQueryString(origQuery);
         req.url = url.format(origUrl);
         return proxyPass(req, res, upstream, this, requestId, false);
       }
@@ -1798,10 +1802,11 @@ export default class DevServer {
       Array.isArray(buildResult.routes) &&
       buildResult.routes.length > 0
     ) {
-      const origUrl = url.parse(req.url || '/', true);
-      delete origUrl.search;
+      const origUrl = url.parse(req.url || '/');
+      const origQuery = parseQueryString(origUrl.search);
       origUrl.pathname = dest;
-      Object.assign(origUrl.query, uri_args);
+      Object.assign(origQuery, query);
+      origUrl.search = formatQueryString(origQuery);
       const newUrl = url.format(origUrl);
       debug(
         `Checking build result's ${buildResult.routes.length} \`routes\` to match ${newUrl}`
@@ -1897,11 +1902,13 @@ export default class DevServer {
         );
 
         // Mix in the routing based query parameters
-        const parsed = url.parse(req.url || '/', true);
-        Object.assign(parsed.query, uri_args);
+        const origUrl = url.parse(req.url || '/');
+        const origQuery = parseQueryString(origUrl.search);
+        Object.assign(origQuery, query);
+        origUrl.search = formatQueryString(origQuery);
         req.url = url.format({
-          pathname: parsed.pathname,
-          query: parsed.query,
+          pathname: origUrl.pathname,
+          search: origUrl.search,
         });
 
         // Add the Vercel platform proxy request headers
@@ -2017,11 +2024,13 @@ export default class DevServer {
         requestId = generateRequestId(this.podId, true);
 
         // Mix the `routes` result dest query params into the req path
-        const parsed = url.parse(req.url || '/', true);
-        Object.assign(parsed.query, uri_args);
+        const origUrl = url.parse(req.url || '/');
+        const origQuery = parseQueryString(origUrl.search);
+        Object.assign(origQuery, query);
+        origUrl.search = formatQueryString(origQuery);
         const path = url.format({
-          pathname: parsed.pathname,
-          query: parsed.query,
+          pathname: origUrl.pathname,
+          search: origUrl.search,
         });
 
         const body = await rawBody(req);

packages/cli/src/util/dev/types.ts

@@ -145,8 +145,8 @@ export interface RouteResult {
   status?: number;
   // "headers": <object of the added response header values>
   headers: HttpHeadersConfig;
-  // "uri_args": <object (key=value) list of new uri args to be passed along to dest >
-  uri_args?: { [key: string]: any };
+  // "query": <object (key=values) of new uri args to be passed along to dest>
+  query?: Record<string, string[]>;
   // "matched_route": <object of the route spec that matched>
   matched_route?: Route;
   // "matched_route_idx": <integer of the index of the route matched>

packages/cli/test/dev/fixtures/vite-dev/.gitignore

@@ -0,0 +1,7 @@
+node_modules
+.DS_Store
+dist
+dist-ssr
+*.local
+.vercel
+!public

packages/cli/test/dev/fixtures/vite-dev/index.html

@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html lang="en">
+  <head>
+    <meta charset="UTF-8" />
+    <link rel="icon" href="/favicon.ico" />
+    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
+    <title>Vite App</title>
+  </head>
+  <body>
+    <div id="app"></div>
+    <script type="module" src="/src/main.js"></script>
+  </body>
+</html>

packages/cli/test/dev/fixtures/vite-dev/package.json

@@ -0,0 +1,16 @@
+{
+  "private": true,
+  "scripts": {
+    "dev": "vite --port $PORT",
+    "build": "vite build",
+    "serve": "vite preview"
+  },
+  "dependencies": {
+    "vue": "3.2.37"
+  },
+  "devDependencies": {
+    "@vitejs/plugin-vue": "1.10.2",
+    "@vue/compiler-sfc": "3.2.37",
+    "vite": "2.9.14"
+  }
+}

packages/cli/test/dev/fixtures/vite-dev/public/favicon.ico

@@ -0,0 +1,22 @@
+<template>
+  <img alt="Vue logo" src="./assets/logo.png" />
+  <HelloWorld msg="Hello Vue 3 + Vite" />
+</template>
+
+<script setup>
+import HelloWorld from './components/HelloWorld.vue'
+
+// This starter template is using Vue 3 experimental <script setup> SFCs
+// Check out https://github.com/vuejs/rfcs/blob/script-setup-2/active-rfcs/0000-script-setup.md
+</script>
+
+<style>
+#app {
+  font-family: Avenir, Helvetica, Arial, sans-serif;
+  -webkit-font-smoothing: antialiased;
+  -moz-osx-font-smoothing: grayscale;
+  text-align: center;
+  color: #2c3e50;
+  margin-top: 60px;
+}
+</style>

packages/cli/test/dev/fixtures/vite-dev/src/App.vue

@@ -0,0 +1,33 @@
+<template>
+  <h1>{{ msg }}</h1>
+
+  <p>
+    <a href="https://vitejs.dev/guide/features.html" target="_blank">
+      Vite Documentation
+    </a>
+    |
+    <a href="https://v3.vuejs.org/" target="_blank">Vue 3 Documentation</a>
+  </p>
+
+  <button type="button" @click="state.count++">count is: {{ state.count }}</button>
+  <p>
+    Edit
+    <code>components/HelloWorld.vue</code> to test hot module replacement.
+  </p>
+</template>
+
+<script setup>
+import { defineProps, reactive } from 'vue'
+
+defineProps({
+  msg: String
+})
+
+const state = reactive({ count: 0 })
+</script>
+
+<style scoped>
+a {
+  color: #42b983;
+}
+</style>

packages/cli/test/dev/fixtures/vite-dev/src/assets/logo.png

@@ -0,0 +1,4 @@
+import { createApp } from 'vue';
+import App from './App.vue';
+
+createApp(App).mount('#app');

packages/cli/test/dev/fixtures/vite-dev/src/components/HelloWorld.vue

@@ -0,0 +1,7 @@
+import { defineConfig } from 'vite';
+import vue from '@vitejs/plugin-vue';
+
+// https://vitejs.dev/config/
+export default defineConfig({
+  plugins: [vue()],
+});