This repository has been archived by the owner. It is now read-only.

Ensure user packages are separated from bundled packages #37

Merged
merged 5 commits into from Oct 31, 2017

Conversation

Projects
None yet
3 participants
@Ben3eeE
Member

Ben3eeE commented Oct 30, 2017

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

The previous check for bundled and user packages was checking for the existence of app.asar. This means it stopped working when we removed the asar. The regex also only worked for paths with forward slashes so it didn't work on Windows.

Alternate Designs

Did not consider any alternative designs

Benefits

Fixes exception reporting so User packages and Bundled packages are separated

Possible Drawbacks

Introduces path as a dependency

Applicable Issues

Fixes #36

/cc: @nathansobo for review

Shoutouts to @50Wliu for helping me through some major git problems and for bouncing ideas while I was being tired and confused.

Show outdated Hide outdated package.json
Show outdated Hide outdated spec/reporter-spec.js
Show outdated Hide outdated lib/reporter.js
Show outdated Hide outdated spec/reporter-spec.js
@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 31, 2017

Contributor

This looks like a small change. They key thing to do here is to link it and synthesize an exception in user package to make sure it goes to bugsnag correctly. If you can confirm it's working, then I'd say 🚢.

Contributor

nathansobo commented Oct 31, 2017

This looks like a small change. They key thing to do here is to link it and synthesize an exception in user package to make sure it goes to bugsnag correctly. If you can confirm it's working, then I'd say 🚢.

Show outdated Hide outdated lib/reporter.js
@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Oct 31, 2017

Member

@nathansobo I tested this by changing alwaysReport to true in the constructor and starting Atom in dev mode with exception-reporting linked.

I then reproduced an exception and found it in bugsnag by searching for my userId.

userPackages contains all packages that I have installed and exception-reporting because it is a linked core package.

bundledPackages contains all core packages that I have activated.

Member

Ben3eeE commented Oct 31, 2017

@nathansobo I tested this by changing alwaysReport to true in the constructor and starting Atom in dev mode with exception-reporting linked.

I then reproduced an exception and found it in bugsnag by searching for my userId.

userPackages contains all packages that I have installed and exception-reporting because it is a linked core package.

bundledPackages contains all core packages that I have activated.

@50Wliu

50Wliu approved these changes Oct 31, 2017

@Ben3eeE Ben3eeE merged commit b4830f9 into master Oct 31, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Ben3eeE Ben3eeE deleted the b3-fix-for-asar branch Oct 31, 2017

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 31, 2017

Contributor

Nice work!! 💥

Contributor

nathansobo commented Oct 31, 2017

Nice work!! 💥

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Oct 31, 2017

Member

Huge shoutouts to @50Wliu for all the help with this. It was a huge help 💯

Member

Ben3eeE commented Oct 31, 2017

Huge shoutouts to @50Wliu for all the help with this. It was a huge help 💯

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.