Skip to content

Commit 2d807d5

Browse files
committed
test: address registry proxy review comments
- forward the original request method and body upstream so npm's POST endpoints (audit bulk advisories) work through the proxy; redirects are only followed for body-less methods since the body streams into the first request - resolve the upstream registry from the project .npmrc in cwd before the user ~/.npmrc, so projects on a custom registry keep resolving from it - remove the --pack temp directory in the normal cleanup paths (wrapper exit and --serve shutdown) instead of leaving it for --kill - document the deliberate limitations: registry env vars are ignored as upstream sources (stale exports from a previous --serve would become the upstream), and HTTP(S)_PROXY tunneling plus authenticated upstreams are out of scope for the environments this tool serves
1 parent d0cbcb5 commit 2d807d5

4 files changed

Lines changed: 89 additions & 46 deletions

File tree

docs/.vitepress/config.mts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ export default extendConfig(
150150
{ text: 'Releases', link: 'https://github.com/voidzero-dev/vite-plus/releases' },
151151
{
152152
text: 'Announcement',
153-
link: 'https://voidzero.dev/posts/announcing-vite-plus-alpha',
153+
link: 'https://voidzero.dev/posts/announcing-vite-plus-beta',
154154
},
155155
{
156156
text: 'Contributing',

docs/.vitepress/theme/components/home/Hero.vue

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
<div class="flex flex-wrap items-center justify-center gap-5">
1616
<a href="/guide" target="_self" class="button button--primary"> Get started </a>
1717
<a
18-
href="https://voidzero.dev/posts/announcing-vite-plus-alpha"
18+
href="https://voidzero.dev/posts/announcing-vite-plus-beta"
1919
target="_blank"
2020
rel="noopener noreferrer"
2121
class="button"

docs/guide/troubleshooting.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
Use this page when something in Vite+ is not behaving the way you expect.
44

55
::: warning
6-
Vite+ is still in alpha. We are making frequent changes, adding features quickly, and we want feedback to help make it great.
6+
Vite+ is still in beta. We are making frequent changes, adding features quickly, and we want feedback to help make it great.
77
:::
88

99
## Supported Tool Versions

packages/tools/src/local-npm-registry.ts

Lines changed: 86 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ import {
4646
type OutgoingHttpHeaders,
4747
type ServerResponse,
4848
} from 'node:http';
49-
import { Agent as HttpsAgent, get as httpsGet } from 'node:https';
49+
import { Agent as HttpsAgent, get as httpsGet, request as httpsRequest } from 'node:https';
5050
import { homedir, tmpdir } from 'node:os';
5151
import path from 'node:path';
5252
import { fileURLToPath } from 'node:url';
@@ -161,32 +161,44 @@ if (!serve && command.length === 0) {
161161
// The script lives at `packages/tools/src/`, so the repo root is three up.
162162
const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..', '..');
163163

164+
// Tracked so the normal cleanup paths remove the packed tarballs; a hard
165+
// kill leaves it behind, which `--kill` sweeps up with the other
166+
// vp-local-registry-* leftovers.
167+
let packedDir: string | undefined;
164168
if (pack && !packagesDir) {
165-
packagesDir = mkdtempSync(path.join(tmpdir(), 'vp-local-registry-pack-'));
166-
await packLocalVitePlusPackages(repoRoot, packagesDir);
169+
packedDir = mkdtempSync(path.join(tmpdir(), 'vp-local-registry-pack-'));
170+
await packLocalVitePlusPackages(repoRoot, packedDir);
171+
packagesDir = packedDir;
167172
}
168173

169174
const manifest: Record<string, unknown> = existsSync('./mock-manifest.json')
170175
? (JSON.parse(readFileSync('./mock-manifest.json', 'utf-8')) as Record<string, unknown>)
171176
: {};
172177

173-
// Proxy through the user's configured registry (e.g. a local mirror in
174-
// `~/.npmrc`) when there is one, so local runs stay as fast as direct
175-
// installs. CI has no user registry config and uses npmjs. Deliberately
176-
// reads only `~/.npmrc` and NOT registry env vars (unlike the CLI's
178+
// Proxy through the configured registry (the project's `.npmrc` in cwd, then
179+
// the user's `~/.npmrc`, e.g. a local mirror) when there is one, so runs stay
180+
// as fast as direct installs and projects that rely on a custom registry keep
181+
// resolving from it. CI has no registry config and uses npmjs. Deliberately
182+
// reads only `.npmrc` files and NOT registry env vars (unlike the CLI's
177183
// getNpmRegistry): a leftover NPM_CONFIG_REGISTRY export from a previous
178-
// `--serve` session would otherwise become this server's own upstream.
184+
// `--serve` session would otherwise become this server's own upstream (the
185+
// https-only guard below rejects such http URLs for the same reason).
186+
// Known out-of-scope for this test tool: HTTP(S)_PROXY tunneling and
187+
// authenticated upstream registries; the environments it serves (repo CI and
188+
// local dev) need neither.
179189
function resolveUpstreamRegistry(): string {
180-
try {
181-
const npmrc = readFileSync(path.join(homedir(), '.npmrc'), 'utf-8');
182-
const registryLine = npmrc.split('\n').find((line) => line.trim().startsWith('registry='));
183-
const registry = registryLine?.split('=')[1]?.trim().replace(/\/+$/, '');
184-
// The proxy fetches with node:https, so only accept https upstreams.
185-
if (registry?.startsWith('https://')) {
186-
return registry;
190+
for (const npmrcPath of [path.resolve('.npmrc'), path.join(homedir(), '.npmrc')]) {
191+
try {
192+
const npmrc = readFileSync(npmrcPath, 'utf-8');
193+
const registryLine = npmrc.split('\n').find((line) => line.trim().startsWith('registry='));
194+
const registry = registryLine?.split('=')[1]?.trim().replace(/\/+$/, '');
195+
// The proxy fetches with node:https, so only accept https upstreams.
196+
if (registry?.startsWith('https://')) {
197+
return registry;
198+
}
199+
} catch {
200+
// no such .npmrc: try the next one
187201
}
188-
} catch {
189-
// no user .npmrc: use the default registry
190202
}
191203
return 'https://registry.npmjs.org';
192204
}
@@ -354,6 +366,13 @@ function proxyToUpstream(req: IncomingMessage, res: ServerResponse): void {
354366
}
355367
res.end(`proxy error: ${error.message}`);
356368
};
369+
// Forward the original method and body: npm-based clients POST to registry
370+
// endpoints too (e.g. the audit bulk advisories). The body can only be
371+
// streamed into the FIRST upstream request, so redirects are followed only
372+
// for body-less methods; a redirect on anything else is forwarded to the
373+
// client as-is (its `location` header survives below).
374+
const method = req.method ?? 'GET';
375+
const bodyless = method === 'GET' || method === 'HEAD';
357376
const fetchUrl = (url: string, redirectsLeft: number) => {
358377
// Tarballs: `accept-encoding: identity` keeps mirrors/CDNs from wrapping
359378
// them in an extra content-encoding layer, which some package managers
@@ -368,34 +387,55 @@ function proxyToUpstream(req: IncomingMessage, res: ServerResponse): void {
368387
? 'identity'
369388
: (req.headers['accept-encoding'] ?? 'identity'),
370389
};
371-
httpsGet(url, { headers, agent: upstreamAgent }, (upstream) => {
372-
const status = upstream.statusCode ?? 502;
373-
if (status >= 300 && status < 400 && upstream.headers.location && redirectsLeft > 0) {
374-
// Draining can still emit 'error' (e.g. the socket resets mid-redirect),
375-
// so guard it here too — otherwise it's uncaught and crashes the server.
376-
upstream.on('error', fail);
377-
upstream.resume();
378-
fetchUrl(new URL(upstream.headers.location, url).toString(), redirectsLeft - 1);
379-
return;
390+
for (const name of ['content-type', 'content-length'] as const) {
391+
if (req.headers[name] !== undefined) {
392+
headers[name] = req.headers[name];
380393
}
381-
const responseHeaders: OutgoingHttpHeaders = {};
382-
// Forward `location` too, so a 3xx we stop following (or one with no
383-
// location to follow) still reaches the client with its redirect target.
384-
for (const name of ['content-type', 'content-encoding', 'content-length', 'location']) {
385-
if (upstream.headers[name] !== undefined) {
386-
responseHeaders[name] = upstream.headers[name];
394+
}
395+
const upstreamRequest = httpsRequest(
396+
url,
397+
{ method, headers, agent: upstreamAgent },
398+
(upstream) => {
399+
const status = upstream.statusCode ?? 502;
400+
if (
401+
status >= 300 &&
402+
status < 400 &&
403+
upstream.headers.location &&
404+
redirectsLeft > 0 &&
405+
bodyless
406+
) {
407+
// Draining can still emit 'error' (e.g. the socket resets mid-redirect),
408+
// so guard it here too — otherwise it's uncaught and crashes the server.
409+
upstream.on('error', fail);
410+
upstream.resume();
411+
fetchUrl(new URL(upstream.headers.location, url).toString(), redirectsLeft - 1);
412+
return;
387413
}
388-
}
389-
// Default a missing content-type (parity with the prior fetch-based proxy)
390-
// so clients that key off it still recognize a proxied tarball.
391-
responseHeaders['content-type'] ??= 'application/octet-stream';
392-
res.writeHead(status, responseHeaders);
393-
// `pipe` does not forward source errors, so listen on the response stream
394-
// directly; otherwise a mid-stream upstream error is uncaught and crashes
395-
// the mock server.
396-
upstream.on('error', fail);
397-
upstream.pipe(res);
398-
}).on('error', fail);
414+
const responseHeaders: OutgoingHttpHeaders = {};
415+
// Forward `location` too, so a 3xx we stop following (or one with no
416+
// location to follow) still reaches the client with its redirect target.
417+
for (const name of ['content-type', 'content-encoding', 'content-length', 'location']) {
418+
if (upstream.headers[name] !== undefined) {
419+
responseHeaders[name] = upstream.headers[name];
420+
}
421+
}
422+
// Default a missing content-type (parity with the prior fetch-based proxy)
423+
// so clients that key off it still recognize a proxied tarball.
424+
responseHeaders['content-type'] ??= 'application/octet-stream';
425+
res.writeHead(status, responseHeaders);
426+
// `pipe` does not forward source errors, so listen on the response stream
427+
// directly; otherwise a mid-stream upstream error is uncaught and crashes
428+
// the mock server.
429+
upstream.on('error', fail);
430+
upstream.pipe(res);
431+
},
432+
);
433+
upstreamRequest.on('error', fail);
434+
if (bodyless) {
435+
upstreamRequest.end();
436+
} else {
437+
req.pipe(upstreamRequest);
438+
}
399439
};
400440
fetchUrl(`${UPSTREAM_REGISTRY}${req.url ?? '/'}`, 5);
401441
}
@@ -458,6 +498,9 @@ function buildRegistryEnv(registry: string): Record<string, string> {
458498
function cleanupRegistryEnv(env: Record<string, string>): void {
459499
rmSync(env.YARN_GLOBAL_FOLDER, { recursive: true, force: true });
460500
rmSync(env.BUN_INSTALL_CACHE_DIR, { recursive: true, force: true });
501+
if (packedDir) {
502+
rmSync(packedDir, { recursive: true, force: true });
503+
}
461504
}
462505

463506
server.listen(0, '127.0.0.1', () => {

0 commit comments

Comments
 (0)