Improve path normalization #39
Conversation
as-cii
force-pushed
the
as-improve-path-normalization
branch
from
06ca06f to
7e5040a
Compare
lib/reporter.js
Outdated
| if (this.isBundledFile(absolutePath)) { | ||
| relativePath = path.relative(this.resourcePath, absolutePath) | ||
| } else { | ||
| const homeDirPath = fs.getHomeDirectory() |
There was a problem hiding this comment.
I think it might make more sense to make the home directory use forward slashes here rather than flip the relative path next.
lib/reporter.js
Outdated
| relativePath = absolutePath | ||
| .replace(/[/]/g, '\\') // Temp switch for Windows home matching | ||
| .replace(homeDirPath.replace(/[/]/g, '\\'), '~') // Remove users home dir | ||
| .replace(/.*(\/(app\.asar|packages\/).*)/, '$1') // Remove everything before app.asar or pacakges |
There was a problem hiding this comment.
pacakges should be packages - might as well correct that - was wrong previously too.
lib/reporter.js
Outdated
| return fileName.indexOf(atom.getLoadSettings().resourcePath) === 0 | ||
| } | ||
| isBundledFile (fileName) { | ||
| return fileName.indexOf(this.resourcePath) === 0 |
There was a problem hiding this comment.
I don't think this ever worked on Windows - (or isTeletypeFile below) as atom.getLoadSettings().resourcePath is going to return an absolute Windows path, e.g. C:\Users\damie\AppData\Local\Atom x64\app-dev\resources\app.asar but we convert the filename into a linux style forward-slash one.
Ditto atom.packages.resolvePackagePath('teletype')
as-cii
force-pushed
the
as-improve-path-normalization
branch
from
0dbd996 to
a880ce6
Compare
The latter indicates the `app.asar` archive location, but stack trace paths don't include the `.asar` extension. Instead, with this commit we use `process.resourcesPath`, which is set to a location that's one folder above `loadSettings.resourcePath`. This allows to correctly identify files that are bundled.
|
This works as expected on macOS (and I assume on Linux too), see:
@damieng: can you do a smoke test on Windows to see if things work properly? Tests pass on AppVeyor, so I assume it will work correctly via a manual test too. Once @damieng confirms this works properly on Windows, I think we should go ahead and 🚢. Thanks everyone for the help on this. |
|
Are there any specific things you want me to try? |
In my comment above I tested that:
I think we should test similar things on Windows and then ship this. What do you think? Any other test you can think of for this pull request is definitely welcome! 👍 |
3 participants
Description of the Change
This pull request contains a few improvements to this package:
/home-directory/foo/barto~/foo/bar, causing further duplication on BugSnag.Alternate Designs
None.
Benefits
Prevents stack traces that logically belong to the same call site from being duplicated on BugSnag.
Possible Drawbacks
Unclear.
Applicable Issues
Fixes #35