Skip to content
This repository has been archived by the owner on Jan 12, 2021. It is now read-only.

Fix --upload-sources stream/buffer logic #43

Merged
merged 4 commits into from
May 23, 2019

Conversation

bengourley
Copy link
Contributor

@bengourley bengourley commented May 23, 2019

During the refactor for #40, the wrong variable name was referenced in getSendableSource() which meant that the --upload-sources logic ceased to work, introducing #42.

While tracking this down, I also noticed that --upload-sources was not compatible with relative paths in the source map.

Also while tracking this down I noticed that the error information included in the API response was not surfaced to the user, so I updated the output there.

To test these fixes, using a similar setup at the OP in #42:

npm i -g react-native-cli
react-native init BugsnagSourceMapUploadTest
cd BugsnagSourceMapUploadTest
react-native bundle \
  --platform ios \
  --dev false \
  --entry-file index.js \
  --bundle-output ios-release.bundle \
  --sourcemap-output ios-release.bundle.map

Make sure you have bugsnag-sourcemaps cloned locally (checkout master) and reference it like so:

../../path-to/bugsnag-sourcemaps/cli.js \
  --api-key=YOUR_API_KEY \
  --app-version 1.11.111 \
  --minified-file ios-release.bundle \
  --source-map ios-release.bundle.map \
  --upload-sources

You should see the following error:

[info] Uploading (ios-release.bundle.map)
[error] Error uploading source maps: Unknown read stream path of type "string"

Now check out this branch (bengourley/upload-sources-fix) and repeat – you should see a successful upload (if you used a valid api key) or an API error (invalid API key, uploading source maps for an existing version etc.), showing that the library sent a reasonable request.

[info] Uploading (ios-release.bundle.map)

or

[error] Error uploading source maps: Error: HTTP status 401 received from upload API
    at res.pipe.body (/Users/bengourley/Development/bugsnag-sourcemaps/lib/request.js:40:21)
    at ConcatStream.<anonymous> (/Users/bengourley/Development/bugsnag-sourcemaps/node_modules/concat-stream/index.js:37:43)
    at ConcatStream.emit (events.js:187:15)
    at finishMaybe (/Users/bengourley/Development/bugsnag-sourcemaps/node_modules/readable-stream/lib/_stream_writable.js:620:14)
    at afterWrite (/Users/bengourley/Development/bugsnag-sourcemaps/node_modules/readable-stream/lib/_stream_writable.js:466:3)
    at process._tickCallback (internal/process/next_tick.js:63:19)

You can also use the example TypeScript project to test out the fix for --upload-sources in conjunction with --directory mode:

  1. Set "inlineSources": false (or comment out that line) in tsconfig.json
  2. npm run build – this will now build without including source content in the map
  3. npm run upload-source-maps – this won't include sources
  4. npm start – sends an error
  5. Check that the error appears in the dashboard, but has no surrounding code
  6. npm run upload-source-maps -- --overwrite --upload-sources re-upload source maps but tell it to include the sources
  7. npm start – sends another error
  8. Verify that the new error has surrounding code (if you are on this branch it should, but on master it won't)

The Object.assign() call only creates a new top-level reference. The nested options.sources key
needs to be replaced, otherwise each upload modifies the same object.
Copy link

@tomlongridge tomlongridge left a comment

Choose a reason for hiding this comment

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

LGTM

@bengourley bengourley merged commit 6d306d5 into master May 23, 2019
@bengourley bengourley deleted the bengourley/upload-sources-fix branch May 23, 2019 13:01
@ryanliszewski
Copy link

Thanks for getting on this so quickly! @bengourley

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants