Repository navigation
Conversation
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed a22d6ee. TypeScript compilation, oxlint, the production bundle, and all 14 new CRAN/loopback HTTP tests passed locally. The packaged file list includes the required webview assets and icons; CI is also green on all three OSes.
One remaining regression is detailed inline: the Fetch-blocked-port workaround covers the R help server, but package discovery from a private CRAN repository on such a port still breaks. I reproduced this against both the PR and its base revision.
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed c01e4dc. The previous CRAN port-10080 finding is fixed: the authenticated repository regression test now passes with redirects and gzip compression. TypeScript compilation, oxlint, the production bundle, and all 16 HTTP/CRAN tests passed locally; the packaged asset list is unchanged, and CI is green on macOS, Linux, and Windows.
One remaining compatibility regression is detailed inline: gzip decompression is stricter than node-fetch's deliberate tolerance for complete HTTP responses whose gzip stream lacks its final trailer. I reproduced the package-discovery difference against the PR's base revision.
| switch (response.headers['content-encoding']?.toLowerCase()) { | ||
| case 'gzip': | ||
| case 'x-gzip': | ||
| body = await decompressGzip(body); |
There was a problem hiding this comment.
[P2] Preserve node-fetch's gzip finish-flush behavior
If a repository server/proxy sends a complete HTTP response containing a flushed gzip stream without the final gzip trailer, this call throws Z_BUF_ERROR: unexpected end of file. node-fetch explicitly used finishFlush: Z_SYNC_FLUSH for gunzip to tolerate these server responses. I reproduced this with a Content-Encoding: gzip package index made using gzipSync(index, { finishFlush: constants.Z_SYNC_FLUSH }), and also with only the eight-byte trailer removed from an otherwise complete gzip body: the base revision discovers privatePackage, but this revision's getPackagesFromCran() silently returns []. The same transport also handles external help pages. Please preserve the old gunzip options (especially finishFlush: Z_SYNC_FLUSH) and add a regression fixture for this case while retaining the invalid-gzip rejection test.
There was a problem hiding this comment.
Thanks for the detailed investigation.
I don't think we should preserve this behavior. Unlike the blocked-port case, this requires a malformed/incomplete gzip stream, and using Z_SYNC_FLUSH would also make us accept genuinely truncated responses and potentially parse partial content.
I don't think node-fetch's permissive handling of malformed gzip should become part of vscode-R's compatibility contract, so I'd prefer to keep the current strict behavior.
There was a problem hiding this comment.
Agreed — keeping strict gzip decoding is a reasonable intentional behavior change. I overstated this compatibility difference as a P2, and I withdraw that finding.
I checked your concern with a 400-package index whose compressed body was cut halfway through, served as a complete HTTP response. The previous node-fetch implementation accepted 191 packages as a successful result, while the current transport rejected the incomplete gzip stream with Z_BUF_ERROR. That supports keeping the strict behavior rather than restoring Z_SYNC_FLUSH.
An explicit truncated-gzip regression fixture would be useful to document this decision, but I do not consider it a merge blocker.
|
@renkun-ken Could you take a look at this? |
renkun-ken
left a comment
There was a problem hiding this comment.
The shared HTTP(S) transport fixes the help-server and private-CRAN blocked-port regressions. After checking the response to the gzip finding, I agree that rejecting incomplete gzip streams is the right tradeoff and withdraw my earlier P2. No remaining actionable findings on c01e4dc.
The revision is unchanged since the previous review, where TypeScript compilation, oxlint, the production build, and all 16 HTTP/CRAN tests passed locally. CI remains green on macOS, Linux, and Windows. Additional verification confirmed that permissive decoding can accept a partial package index, while this transport rejects it.
Summary
Reduce VSIX size and remove unnecessary runtime dependencies.
The VSIX size is reduced from approximately 3.1 MB to 1.2 MB.
Follow-up for v4
Some of the compatibility preserved here, especially package discovery from authenticated/private repositories and repositories using unusual ports, exists largely because it happened to work with the previous node-fetch implementation. It is unclear whether these cases are actively used or should be considered part of vscode-R's long-term compatibility contract.
This PR preserves the existing behavior to avoid introducing regressions in the 3.x series. For vscode-R 4.0, we should reconsider whether the extension needs to maintain its own general-purpose HTTP transport for these cases, or whether package discovery can rely on standard Fetch behavior and leave more specialized repository access to R itself.