Adopt to new web platform specifier extension resource url format - #758
Closed
amvanbaren wants to merge 1 commit into
Closed
amvanbaren wants to merge 1 commit into
amvanbaren wants to merge 1 commit into
Conversation
Added target to unpkg endpoint Added unit tests
amvanbaren
force-pushed
the
feature/issue-744
branch
from
June 20, 2023 10:03
72e9efd to
5e75785
Compare
Contributor
|
Reimplemented this on current Confirmed the problem is still live. The second is the URL a web VS Code sends, so that extension's resources do not load in vscode.dev at all. The new PR follows your design, including the two details that are easiest to miss:
It came out smaller than yours because the plumbing has since become target-aware: #2183 credits you and closes this. Thanks for the original diagnosis; it was right, it just needed re-doing against a file that had moved on. |
netomi
added a commit
that referenced
this pull request
Sep 9, 2026
… for VS Code encodes the target platform into the version segment when it resolves an extension's resources - `<version>+<target>`, `web` for a web extension (microsoft/vscode#180525). The unpkg endpoint never learned to read it, so such a request 404s: /vscode/unpkg/BroadcomMFD/hlasm-language-support/1.23.0/... 200 /vscode/unpkg/BroadcomMFD/hlasm-language-support/1.23.0+web/... 404 That extension publishes 1.23.0 for both `universal` and `web`, and the second URL is the one a web VS Code sends, so its resources do not load at all. Without a target the endpoint also could not have distinguished them: it passed null down to the download lookup, which then matched whichever version came first. The plumbing already took a target platform - WebResourceService's getExtensionDownload and browseExtensionPackage both have the parameter, and browseExtensionPackage used it only to compose an error message - so this is mostly a matter of resolving one at the edge and passing it along. The target comes from `?target=`, or from a `+<target>` suffix on the version when that is absent. The suffix is only taken when it names a platform: a version may legitimately carry semver build metadata (`1.2.3+build.5`), which SemanticVersion's own regex permits, and that belongs to the version. Directory listings re-append the target to the version in the URLs they hand back. Without that, walking into a subdirectory silently drops to whichever version matches first, which is the same bug one level down. UpstreamVSCodeService forwards the target as a query parameter, so a mirror passes it on rather than resolving it against its own registry. Reimplements #758, which reported and fixed this in June 2023 - credit to @amvanbaren. That PR cannot be rebased: it is written against ResponseEntity<byte[]> and a codebase without WebResourceService, and the repository method it adds is now redundant since findFileByType already resolves a target-specific version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
netomi
added a commit
that referenced
this pull request
Sep 10, 2026
… for VS Code encodes the target platform into the version segment when it resolves an extension's resources - `<version>+<target>`, `web` for a web extension (microsoft/vscode#180525). The unpkg endpoint never learned to read it, so such a request 404s: /vscode/unpkg/BroadcomMFD/hlasm-language-support/1.23.0/... 200 /vscode/unpkg/BroadcomMFD/hlasm-language-support/1.23.0+web/... 404 That extension publishes 1.23.0 for both `universal` and `web`, and the second URL is the one a web VS Code sends, so its resources do not load at all. Without a target the endpoint also could not have distinguished them: it passed null down to the download lookup, which then matched whichever version came first. The plumbing already took a target platform - WebResourceService's getExtensionDownload and browseExtensionPackage both have the parameter, and browseExtensionPackage used it only to compose an error message - so this is mostly a matter of resolving one at the edge and passing it along. The target comes from `?target=`, or from a `+<target>` suffix on the version when that is absent. The suffix is only taken when it names a platform: a version may legitimately carry semver build metadata (`1.2.3+build.5`), which SemanticVersion's own regex permits, and that belongs to the version. Directory listings re-append the target to the version in the URLs they hand back. Without that, walking into a subdirectory silently drops to whichever version matches first, which is the same bug one level down. UpstreamVSCodeService forwards the target as a query parameter, so a mirror passes it on rather than resolving it against its own registry. Reimplements #758, which reported and fixed this in June 2023 - credit to @amvanbaren. That PR cannot be rebased: it is written against ResponseEntity<byte[]> and a codebase without WebResourceService, and the repository method it adds is now redundant since findFileByType already resolves a target-specific version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
netomi
added a commit
that referenced
this pull request
Sep 25, 2026
… for VS Code encodes the target platform into the version segment when it resolves an extension's resources - `<version>+<target>`, `web` for a web extension (microsoft/vscode#180525). The unpkg endpoint never learned to read it, so such a request 404s: /vscode/unpkg/BroadcomMFD/hlasm-language-support/1.23.0/... 200 /vscode/unpkg/BroadcomMFD/hlasm-language-support/1.23.0+web/... 404 That extension publishes 1.23.0 for both `universal` and `web`, and the second URL is the one a web VS Code sends, so its resources do not load at all. Without a target the endpoint also could not have distinguished them: it passed null down to the download lookup, which then matched whichever version came first. The plumbing already took a target platform - WebResourceService's getExtensionDownload and browseExtensionPackage both have the parameter, and browseExtensionPackage used it only to compose an error message - so this is mostly a matter of resolving one at the edge and passing it along. The target comes from `?target=`, or from a `+<target>` suffix on the version when that is absent. The suffix is only taken when it names a platform: a version may legitimately carry semver build metadata (`1.2.3+build.5`), which SemanticVersion's own regex permits, and that belongs to the version. Directory listings re-append the target to the version in the URLs they hand back. Without that, walking into a subdirectory silently drops to whichever version matches first, which is the same bug one level down. UpstreamVSCodeService forwards the target as a query parameter, so a mirror passes it on rather than resolving it against its own registry. Reimplements #758, which reported and fixed this in June 2023 - credit to @amvanbaren. That PR cannot be rebased: it is written against ResponseEntity<byte[]> and a codebase without WebResourceService, and the repository method it adds is now redundant since findFileByType already resolves a target-specific version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
netomi
added a commit
that referenced
this pull request
Sep 25, 2026
… for (#2183) * fix: resolve extension resources for the target platform VS Code asks for VS Code encodes the target platform into the version segment when it resolves an extension's resources - `<version>+<target>`, `web` for a web extension (microsoft/vscode#180525). The unpkg endpoint never learned to read it, so such a request 404s: /vscode/unpkg/BroadcomMFD/hlasm-language-support/1.23.0/... 200 /vscode/unpkg/BroadcomMFD/hlasm-language-support/1.23.0+web/... 404 That extension publishes 1.23.0 for both `universal` and `web`, and the second URL is the one a web VS Code sends, so its resources do not load at all. Without a target the endpoint also could not have distinguished them: it passed null down to the download lookup, which then matched whichever version came first. The plumbing already took a target platform - WebResourceService's getExtensionDownload and browseExtensionPackage both have the parameter, and browseExtensionPackage used it only to compose an error message - so this is mostly a matter of resolving one at the edge and passing it along. The target comes from `?target=`, or from a `+<target>` suffix on the version when that is absent. The suffix is only taken when it names a platform: a version may legitimately carry semver build metadata (`1.2.3+build.5`), which SemanticVersion's own regex permits, and that belongs to the version. Directory listings re-append the target to the version in the URLs they hand back. Without that, walking into a subdirectory silently drops to whichever version matches first, which is the same bug one level down. UpstreamVSCodeService forwards the target as a query parameter, so a mirror passes it on rather than resolving it against its own registry. Reimplements #758, which reported and fixed this in June 2023 - credit to @amvanbaren. That PR cannot be rebased: it is written against ResponseEntity<byte[]> and a codebase without WebResourceService, and the repository method it adds is now redundant since findFileByType already resolves a target-specific version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: cover upstream target forwarding and document the browse 400 case UpstreamVSCodeServiceTest still had a call to browse() with the old 4-argument signature, which no longer compiled once targetPlatform was added to the interface. Fix it and add a test that actually exercises the new upstream ?target= forwarding, since the existing calls all passed null and never covered that branch. Also document the second 400 cause (an unsupported target platform) on the browse endpoint's OpenAPI response, alongside the existing built-in-namespace case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: resolve the exact version before reading a target off its suffix A published version's own semver build metadata can itself name a platform, e.g. 0.16.6+web. The suffix-stripping added for VS Code's <version>+<target> browse requests read that as version 0.16.6 on target web, so a universal build actually published under 0.16.6+web could never be reached. Try the version as given first; only fall back to splitting off a target suffix once that lookup comes back empty. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * perf: try the target-suffix reading first, only fall back to the literal version Checking the version exactly as given before the split reading meant every ordinary <version>+<target> request - the case VS Code actually sends, and the reason this suffix handling exists - paid for a doomed lookup before the one that succeeds. Swap the order: the split reading runs first, and the literal version is only tried if that misses, so the common case stays a single lookup. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: keep an explicit universal target in listed URLs when it's needed Browsing a version whose own suffix looks like a target (e.g. 0.16.6+web) with an explicit ?target=universal resolved correctly, but the returned directory listing dropped the target for universal builds. Following one of those URLs re-enters with no target parameter, so the split reading takes over and misreads the suffix as the target - resolving the wrong build. Only omit the target from listed URLs when the version's own suffix isn't itself ambiguous; keep it otherwise, universal included. Factor the version-suffix check into TargetPlatform.targetInVersionSuffix so VSCodeAPI's split reading and this share one implementation. Also fix a stale test name/comment left over from reordering the split vs. exact-version lookup in the previous commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: document allowable target values and cover invalid-target rejection The browse endpoint's new target parameter validates against the same set of platforms as targetPlatform elsewhere in this class, but was missing the matching allowableValues schema, so the generated OpenAPI contract didn't expose it. Add it, and add a regression test for the existing 400 rejection, mirroring the coverage the other target-platform parameters already have. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #744
Added target to unpkg endpoint
Added unit tests