Repository navigation
[SDK-778] Support React Native bundles and source maps in upload-sourcemaps - #43
Conversation
upload-sourcemaps only scanned a directory for *.js files and only found maps through a sourceMappingURL directive. React Native bundles are main.jsbundle / index.android.bundle, and Hermes bytecode bundles carry no directive, so RN users got "[files:] []" or "map not found" (#16). - <path> can now be a single bundle file or a single .map file; a .map is uploaded for the bundle it is named after (main.jsbundle.map -> main.jsbundle), which covers Hermes builds where the map lives in a different directory from the bundle. - Directory scans also pick up .jsbundle and .bundle files. - RN bundles without a directive fall back to <bundle>.map next to them. - mappedFile (used by --next) now resolves relative to the bundle's own directory instead of the scan root, which was wrong for nested files. - Warn when nothing is found at <path>. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rollbar-circleci-machine
left a comment
There was a problem hiding this comment.
AI Agent Review (openai, openai-astra)
Review of #43: React Native bundles and maps in upload-sourcemaps
The scanner changes are small and hold up against the real code:
- Single-file and
.maptargets.targetIsFileandbasePath(src/sourcemaps/scanner.js:19-21) give a bare bundle or map name asfileName. A.maptarget reuses itself as the map (scanner.js:55-57).Uploader.minifiedUrl(src/sourcemaps/uploader.js:72-74) then produceshttp://reactnativehost/<bundle>as the new uploader tests expect. mappedFile. It is now built from the bundle's own directory (scanner.js:72). That matches howloadMapDatareads the map (scanner.js:82-83), so the--nextzip (src/sourcemaps/signed-url-uploader.js:33) now finds maps for nested files instead of failing to read them.nodir. The installedglobis 7.2.3 (node_modules/glob/package.json:5, from the^7.1.6range inpackage.json:17), which supportsnodir(node_modules/glob/README.md:264). As a side benefit, directories whose names end in.jsare no longer handed toparseMapPath.sourceMappingURLflag. It is now onlytruewhen a directive was actually found. The only readers are tests (test/sourcemaps/uploader.test.js:154,test/sourcemaps/scanner.test.js).- Web builds. The adjacent-map fallback only applies to
.jsbundleand.bundle(scanner.js:63-65), so.jsbehaviour is unchanged as the description says.
Two low-severity findings:
- In directory mode, a React Native bundle keeps its subdirectory in the uploaded URL. That doesn't match the
http://reactnativehost/<bundle name>frames the new README section describes. - The new CLI test's
'valid'assertion also matches themap not validoutput.
Not verified. These claims come from the description and refer to repos that aren't checked out here:
- rollbar-react-native rewrites frames to
reactnativehost. - mox/moxapi accept the upload.
- nginx limits.
The description also says a real upload and --next with React Native maps were not tested. Nothing in the diff shows that the tests or lint pass.
Outside the diff: README line 26 still says "Upload source maps recursively from a directory." That is now incomplete, since path can also be a file.
React Native frames are http://reactnativehost/<bundle name> with no build directory (rollbar.js rewriteFilenamePatterns), so a bundle found in a subdirectory of <path> now uploads under its basename instead of its relative path. Warn when more than one validated map would upload under the same name, which a whole Android build folder can produce. Also make the CLI test assert the exact `valid` line, since `map not valid` contains the same substring, and update the README description of <path>. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rollbar-circleci-machine
left a comment
There was a problem hiding this comment.
AI Agent Review (openai, openai-astra)
Review: React Native bundle and source map support in upload-sourcemaps
I read the changed src/sourcemaps/scanner.js and the code that consumes its output. src/sourcemaps/command.js:54-85 passes scanner.files to Uploader, which builds minified_url from fileName (src/sourcemaps/uploader.js:63-74), or to SignedUrlUploader, which zips mappedFile (src/sourcemaps/signed-url-uploader.js:30-37). I also read the new fixtures and tests.
What checks out
- The single-file target (
scanner.js:19-21,:200-203) and the.map→ bundle-name mapping (scanner.js:73-75,:218) givefileNamevalues ofmain.jsbundleandindex.android.bundle. Withnew URL(fileName, baseUrl)(uploader.js:73) these becomehttp://reactnativehost/<bundle>, whether or not the prefix has a trailing slash. - The adjacent-map fallback only runs for
.jsbundle/.bundle(scanner.js:81-84), so.jsbehaves as before. The angular9 assertions that directive-less.jsfiles stay unmapped are unchanged (test/sourcemaps/scanner.test.js:92-94). nodiris supported by the pinnedglob7.2.3 (node_modules/glob/package.json:5,node_modules/glob/common.js:68). It also stops a directory named*.jsfrom reachingreadFileSync.mappedFileis now resolved from the bundle's own directory (scanner.js:90), which fixes nested web builds in the--nextzip.zipFilesstores entries by basename (signed-url-uploader.js:33), so existing zip behaviour for top-level maps is unchanged.warnDuplicateNamesonly counts validated files, and only React Native bundles are flattened, so web builds can't trigger it by accident.- The CLI dry-run test's trimmed
main.jsbundle.map/validlines match whatOutput.formatproduces for an empty label (src/common/output.js:39-50).
Finding (low): mappedFile uses path.join while loadMapData reads the map with path.resolve. A sourceMappingURL that is an absolute path validates, but is then left out of the --next zip. This predates the PR, but the PR rewrote this exact line.
Not verified
- The server-side claims in the description: mox
frame_can_be_source_mappedacceptingreactnativehostframes, the moxapi/api/1/sourcemapfields, and the nginx size limits. Those repositories are not checked out here. - The claim that rollbar-react-native rewrites frames to
http://reactnativehost/<bundle>. - The description references issue #16 and ticket SDK-778, not a dependent pull request, so there is no dependency to confirm.
- I did not run the test suite or lint. The stated "57 passing" is the author's claim, not something this diff proves.
Minor note (no finding): a missing path still exits 0 after the new warning. CI jobs that point at the wrong path will still pass, just more visibly.
mappedFile was built with path.join while loadMapData read the map with path.resolve, so an absolute sourceMappingURL validated but was then left out of the --next zip. mappedFile now keeps an absolute directive as is, and loadMapData reads mappedFile, so the two can't drift again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rollbar-circleci-machine
left a comment
There was a problem hiding this comment.
AI Agent Review (openai, openai-astra)
What I checked: src/sourcemaps/scanner.js against its callers (src/sourcemaps/command.js, uploader.js, signed-url-uploader.js, requester.js), plus the new tests and fixtures. I did not run the tests, so the stated 58 passing is not confirmed here.
- Map path (
mappedFile): the code is correct.mappedFileis now resolved relative to the bundle's own directory (scanner.js:91), andloadMapDatareads that same path (scanner.js:102). For relative directives this matches the oldpath.resolve(dirname, mapPathName)read. Absolute directives behave as before during validation. The--nextzip (signed-url-uploader.js:33) now gets the real nested map path, where before it gottargetPath/<basename>, a file that did not exist. - Directory and single-file scans:
glob.sync(..., { nodir: true })is supported by the pinned glob 7.2.3 (node_modules/glob/common.js:68). For a single-file<path>, the base directory is the file's own directory (scanner.js:21), sofileNameis the basename. A.maptarget drops the.mapsuffix (scanner.js:218). React Native bundles found in subdirectories upload under their basename (scanner.js:222-224). - Web builds: the adjacent-map fallback is limited to
.jsbundle/.bundle(scanner.js:81), so web.jsbehaviour is unchanged. The existing angular9 and react16 assertions still apply. - Tests: the new tests' expected paths agree with how
path.joinnormalises./test/...paths. CI runs Node 18 and later (.github/workflows/node.js.yml:36-45), sofs.rmSyncin the new test is available. - Not verified: the server-side claims (that rollbar-react-native rewrites frames to
reactnativehost, mox's frame matching, moxapi's size limits) refer to code that is not checked out here. - One gap: the PR description says the README tells React Native users to use the standard upload rather than
--next. The new React Native section does not say that (see finding).
The server side of --next (ingestion's SourceMapProcessor._map_files) uploads each zip entry for <baseUrl><entry name without .map>. Entries were named after the map file's basename, so a React Native bundle's bundle-name rule, a nested web build's subdirectory, and a map named differently from its bundle never reached that path. Name each entry <fileName>.map so --next uploads every map for the same minified_url as the standard upload, and say so in the README. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AI Agent Review (openai, openai-astra)SummaryThis PR lets What I verified
Notes (not findings)
|
Rollbar's React Native docs generate the map with `react-native bundle --sourcemap-output sourcemap.ios.js`, which is the exact input in #16. The scanner only recognized a standalone map by a `.map` extension, so that file was treated as a bundle and reported `map not found`. A single-file <path> is now recognized as a source map by its content as well as by `.map`. Its name can't say which bundle it belongs to, so the new `--bundle-name` option sets the upload name (minified_url is <url-prefix><bundle-name>). Without it the CLI reports an error naming the option. A source map found during a directory scan gets a warning that points to the same flow instead of `map not found`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AI Agent Review (openai, openai-astra)Review: React Native bundles and source maps in
|
Fixes #16. Linear: SDK-778.
What changed
upload-sourcemapscouldn't handle React Native builds. It only globbed<path>/**/*.jsand only found maps through a//# sourceMappingURLdirective. RN bundles are namedmain.jsbundle(iOS) andindex.android.bundle(Android), and Hermes bytecode bundles don't have a directive at all. That produced the two outputs in #16:[files:] []when given a file, andmap not foundwhen given a directory. The reporter's map issourcemap.ios.js, the--sourcemap-outputname used in Rollbar's React Native docs. That name doesn't end in.mapand doesn't say which bundle the map belongs to.In
src/sourcemaps/scanner.js:<path>can now be a single bundle file or a single.mapfile, not only a directory. A.mapis uploaded for the bundle it's named after:index.android.bundle.mapgoes tominified_url = <url-prefix>index.android.bundle. This handles Hermes, where the RN Gradle plugin writes the map tobuild/generated/sourcemaps/react/release/and the bundle somewhere else.<path>also counts as a source map when its content is one (a JSON object withversionandmappings/sections), sosourcemap.ios.jsfrom the docs is recognized. The name doesn't say which bundle the map is for, and Metro writes nofilefield, so the new--bundle-nameoption sets it:upload-sourcemaps sourcemap.ios.js --bundle-name main.jsbundle --url-prefix http://reactnativehostuploads forhttp://reactnativehost/main.jsbundle. Without the flag, the CLI reports an error naming it instead ofmap not found.--bundle-namealso overrides the name derived from a.mapfile and is refused when<path>is a directory. A source map found during a directory scan, which is the second command in Is react native supported? Sourcemaps not found #16, warns that it should be passed as<path>with--bundle-name..jsbundleand.bundlefiles (nodir, so macOS*.bundledirectories are ignored). An RN bundle found in a subdirectory uploads under its bundle name, not its relative path, because rollbar.js's defaultrewriteFilenamePatternsstrip everything before the bundle name. When more than one validated map would upload under the same name, which a whole Androidbuild/folder can produce, the CLI warns..jsbundle/.bundle) with no directive fall back to<bundle>.mapin the same directory. I kept this fallback away from.json purpose so web builds behave exactly as before. The angular9 fixture has directive-less.jsfiles with adjacent maps, and its tests still expect those not to be uploaded.mappedFile, used by the--nextzip, is now resolved relative to the bundle's own directory. It used to be resolved relative to the scan root, which was wrong for nested files (for examplebuild/static/js/x.jsgavebuild/x.js.map) and would have been wrong for a single-file<path>. An absolutesourceMappingURLis kept as is, and the map is validated from that samemappedFile. Before, an absolute directive validated but was then left out of the zip.<path>, the CLI now warns instead of silently printing[files:] [].In
src/sourcemaps/signed-url-uploader.js, the--nextzip now names each map<fileName>.mapinstead of the map's basename. Its consumer, ingestion'sSourceMapProcessor._map_files, uploads each entry for<baseUrl><entry name without .map>. So--nextnow gives every map the sameminified_urlas the standard upload, including RN bundles found in subdirectories, nested web build paths (static/js/x.js, which used to be flattened tox.js), and maps named differently from their bundle.The README has a new React Native section: use
--url-prefix http://reactnativehost/, and either name the map after the bundle or pass--bundle-name. It includes the exact command for the docs'sourcemap.ios.js/sourcemap.android.jsoutputs.--nextis documented as uploading for the same URLs.Server side
Nothing to change:
http://reactnativehost/<bundle>(rollbar.jssrc/react-native/transforms.js), and mox'sframe_can_be_source_mappedexplicitly acceptsreactnativehostframes whatever the extension (mox/lib/source_map.py).POST /api/1/sourcemap(moxapisrc/routes/sourcemap.js) only needsminified_url,versionandsource_map. The bundle itself is never uploaded./api/1/sourcemap, and moxapi's multipart parser doesn't cap file parts. The reporter's 18 MB map is fine. The client-side axios body cap is alreadyInfinityas of 0.3.0.Validation
npm test: 69 passing (47 before), 1 pending (pre-existing).npm run lintis clean.test/fixtures/builds/react-native/: an iOSmain.jsbundlewith a directive, and a Hermes-styleindex.android.bundlewith no directive and an adjacent map.test/fixtures/builds/react-native-sourcemap/sourcemap.ios.jsis a valid map named as in the docs, with nofilefield.--bundle-nametests: the scanner with and without the flag, overriding a.mapname, the directory-scan warning, refusing a directory, an uploader test assertingminified_urlishttp://reactnativehost/main.jsbundlewith the Is react native supported? Sourcemaps not found #16 prefix (no trailing slash), and two CLI dry-run tests. All 8 fail without the change. I also ran both Is react native supported? Sourcemaps not found #16 commands by hand against asourcemap.ios.js: before the change, both printmap not found; after it, they print the new error and warning, and--bundle-name main.jsbundlevalidates the map..mapfile, nested and absolute-directivemappedFileresolution, and a missing path. Uploader tests assertminified_urlishttp://reactnativehost/main.jsbundle/http://reactnativehost/index.android.bundle, for single files and for a directory scan with the bundles in subdirectories. A unit test covers the duplicate-name warning. There's also a CLI dry-run test with a single bundle file, which matches the exactvalidline. I checked that it fails if validation fails.zipFilestests cover zip entry names for an RN directory scan, the nested react16 build, and a map named differently from its bundle. The last two fail on the old basename naming._unzip_source_map/_get_manifest/_map_fileson zips the CLI built from the fixtures. They produced//reactnativehost/index.android.bundle,//reactnativehost/main.jsbundleand//example.com/static/js/<chunk>.js.🤖 Generated with Claude Code