Skip to content

refactor: reduce VSIX assets and runtime dependencies - #1849

Merged
eitsupi merged 4 commits into
REditorSupport:mainfrom
eitsupi:chore/reduce-vsix-and-runtime-dependencies
Oct 8, 2026
Merged

eitsupi merged 4 commits into
REditorSupport:mainfrom
eitsupi:chore/reduce-vsix-and-runtime-dependencies

Conversation

@eitsupi

@eitsupi eitsupi commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Summary

Reduce VSIX size and remove unnecessary runtime dependencies.

  • Remove obsolete screenshots and exclude unused assets from the package.
  • Bundle only the R grammar from highlight.js.
  • Replace fs-extra with Node built-ins while preserving existing filesystem behavior.
  • Replace node-fetch with a shared Node HTTP(S) text transport for R help and CRAN package discovery.
  • Preserve existing behavior for Basic Auth repository URLs, redirects, compressed responses, and repositories using ports blocked by the Fetch API.
  • Preserve external Windows FAQ and manual redirects from R's dynamic help server.

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.

@eitsupi
eitsupi requested review from Fred-Wu and renkun-ken October 7, 2026 23:18
@eitsupi eitsupi changed the title chore: reduce VSIX assets and runtime dependencies refactor: reduce VSIX assets and runtime dependencies Oct 7, 2026
@eitsupi
eitsupi marked this pull request as ready for review October 7, 2026 23:27
Comment thread src/helpViewer/helpProvider.ts Outdated
@eitsupi
eitsupi marked this pull request as draft October 7, 2026 23:48
@eitsupi
eitsupi marked this pull request as ready for review October 8, 2026 00:37
@eitsupi
eitsupi requested a review from Fred-Wu October 8, 2026 00:48
@eitsupi eitsupi added this to the 3.2.0 milestone Oct 8, 2026

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/helpViewer/fetch.ts Outdated
@eitsupi
eitsupi marked this pull request as draft October 8, 2026 02:31
@eitsupi
eitsupi marked this pull request as ready for review October 8, 2026 02:46
@eitsupi
eitsupi requested a review from renkun-ken October 8, 2026 02:46

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/helpViewer/http.ts
switch (response.headers['content-encoding']?.toLowerCase()) {
case 'gzip':
case 'x-gzip':
body = await decompressGzip(body);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eitsupi
eitsupi requested a review from renkun-ken October 8, 2026 03:43
@eitsupi

eitsupi commented Oct 8, 2026

Copy link
Copy Markdown
Member Author

@renkun-ken Could you take a look at this?

@renkun-ken renkun-ken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eitsupi
eitsupi merged commit e61ff7e into REditorSupport:main Oct 8, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants