Open
Conversation
There was a problem hiding this comment.
Pull request overview
Updates JS source map discovery to pair bundles with their referenced source maps by parsing sourceMappingURL directives, aligning behavior with the TC39 ECMA-426 “without parsing” approach.
Changes:
- Add
ExtractSourceMappingURL,ResolveBundlePaths, and a newResolveSourceMapPathsthat returns bundle+map pairs. - Update JS upload flow to use bundle/map pairs and handle direct
.mappath inputs. - Expand unit/feature fixtures and scenarios to reflect bundle-driven map resolution.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/upload/js.go |
Implements sourceMappingURL extraction and bundle→map pairing; updates upload flow accordingly. |
test/upload/js_test.go |
Adds tests for sourceMappingURL extraction and bundle discovery; updates existing sourcemap path test to assert bundle/map pairs. |
features/js/fixtures/js-multiple-maps/dist/other.js |
Updates fixture bundle to reference the new map filename. |
features/js/fixtures/js-multiple-maps/dist/other-123.js.map |
Adds new sourcemap fixture referenced by other.js. |
features/cli/exclude-option.feature |
Updates exclude patterns to match the renamed fixture sourcemap. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+344
to
+352
|
|
||
| // Try to find bundle by stripping .map suffix | ||
| withoutSuffix, found := strings.CutSuffix(sourceMapPath, ".map") | ||
| if found && utils.FileExists(withoutSuffix) { | ||
| logger.Info(fmt.Sprintf("Automatically using the bundle at path %s based on stripping the .map suffix.", withoutSuffix)) | ||
| return []SourceMapBundle{{BundlePath: withoutSuffix, SourceMapPath: sourceMapPath}}, nil | ||
| } | ||
| // If no bundle found, return empty bundle path (bundle is optional) | ||
| return []SourceMapBundle{{BundlePath: "", SourceMapPath: sourceMapPath}}, nil |
Member
Author
There was a problem hiding this comment.
@joshedney bump! I think this seems sensible but just wanted your input
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
lemnik
approved these changes
Mar 18, 2026
joshedney
approved these changes
Mar 18, 2026
Member
Author
|
@tomlongridge requested review for some error wording |
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
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.
Goal
Updates JS source map discovery to pair bundles with their referenced source maps by parsing sourceMappingURL directives, aligning behavior with the TC39 ECMA-426 “without parsing” approach.
Changeset
Testing
Expand unit/feature fixtures and scenarios to reflect bundle-driven map resolution.