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

Add --directory mode #40

Merged
merged 11 commits into from
May 22, 2019
Merged

Add --directory mode #40

merged 11 commits into from
May 22, 2019

Conversation

bengourley
Copy link
Contributor

@bengourley bengourley commented May 16, 2019

Tools that don't end up bundle JS into a single output file can leave many source maps on the filesystem (e.g. Babel, TypeScript). The added --directory flag can be used to tell bugsnag-sourcemaps to search the filesystem from a given path and upload every source map that it
finds.

I made quite a substantial refactor to the library – splitting out the large index.js into smaller modules. Other than that – refactor-wise the scope of the changes were on a "needed to facilicate this new feature" basis.

In order to make the listr module output a reasonable amount of information to the CLI user, it would have necessitated tighter coupling between the uploader and the CLI and a further substantial refactor, so I removed that and implemented a simple logger instead. For this reason the CLI UI will appear very different to users. This may be a concern, but I felt it necessary for the bulk upload to show more than this amount of information:

image

Bulk upload outputs this kind of info:
image

Errors look like this:
image

Testing

To facilitate manually testing the new mode I've added two kinds of Node project to the examples directory: TypeScript and Babel. As you'll see after running the build on each one, TypeScript creates a source map alongside each source/output file in the tree whereas Babel copies the src structure and mimics it in the output lib.

TypeScript

Be sure to replace the API key in examples/typescript/package.json and examples/typescript/src/services/bugsnag.ts before you begin.

cd examples/typescript
npm run build
npm run upload-source-maps
node src/app.js

Babel

Be sure to replace the API key in examples/babel/package.json and examples/babel/src/services/bugsnag.js before you begin.

cd examples/babel
npm run build
npm run upload-source-maps
node lib/app.js

You can run the local developement bugsnag-sourcemaps command from either example using a relative path (per the package.json scripts): ../../cli.js upload --api-key --overwrite etc.

With each project, you should be able to see errors in the dashboad being successfully symbolicated – showing the original stacktraces.

No functionality was changed here, just splitting it up to make it easy to work on.
Tools that don't end up bundle JS into a single output file can leave many source maps on the
filesystem (e.g. Babel, TypeScript). The added --directory flag can be used to tell
bugsnag-sourcemaps to search the filesystem from a given path and upload every source map that it
finds.
@Cawllec
Copy link
Contributor

Cawllec commented May 17, 2019

It's not entirely clear which changes are the --directory implementation, and which are refactoring. Would it be possible to split this into two PRs, one for the refactor (or whichever change came first) and a second for the directory change?

@bengourley
Copy link
Contributor Author

The commits are discreet chunks of work so you should be able to see from that? I can separate into a different PR if necessary.

I've updated the PR description adding directions on how to test the new feature.

Copy link
Contributor

@Cawllec Cawllec left a comment

Choose a reason for hiding this comment

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

Ok, this looks good to me. The majority of the changes are moving methods from index.js into discrete modules, with a few changes to use graceful-fs over built-in fs. The upload -> uploadOne and uploadMany makes sense, as well as the options in the README and cli.js (which make up the --directory change.

I've also tested using the attached examples, and can confirm it works as described with both an empty --directory option and with an explicit path there (i.e. --directory lib/

I've left one comment on the readme, and I think it could do with a couple of things, namely:

  • Unit tests for the upload, uploadOne, & uploadMany (stubbing sendRequest, how difficult would this be?)
  • README's for the example apps.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
bengourley and others added 2 commits May 17, 2019 16:14
@bengourley
Copy link
Contributor Author

with a few changes to use graceful-fs over built-in fs

graceful-fs was in use before. If it's used in places it wasn't before, that's unintentional?

@Cawllec
Copy link
Contributor

Cawllec commented May 17, 2019

You are correct, I read it wrong initially.

@bengourley
Copy link
Contributor Author

Added readmes for the examples. I do want to add unit tests but I wanted to get it queued up for review first. Will add tests on Monday.

Copy link
Contributor

@Cawllec Cawllec left a comment

Choose a reason for hiding this comment

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

Furthering my previous review, LGTM and tests pass :)

@bengourley bengourley merged commit aca2088 into master May 22, 2019
@kattrali kattrali deleted the directory-mode branch October 16, 2020 11:18
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