Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix source maps uploaded from Windows #60

Merged
merged 3 commits into from
May 6, 2021
Merged

Fix source maps uploaded from Windows #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69dafe0
Add windows to CI
imjoehaines May 5, 2021
14b7094
Remove leading Windows path separators too
imjoehaines May 5, 2021
2074cb1
Always use '/' as path separator in minifiedUrl
imjoehaines May 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/qa.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ on: [ push, pull_request ]

jobs:
test:

runs-on: ubuntu-latest
runs-on: ${{ matrix.os }}

strategy:
fail-fast: false
matrix:
node-version: [10.x, 12.x, 14.x]
steps:
os: [ubuntu-20.04, windows-2019]

steps:
- uses: actions/checkout@v2

- name: Use Node.js ${{ matrix.node-version }}
Expand Down
4 changes: 2 additions & 2 deletions src/transformers/StripProjectRoot.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,6 @@ function strip (sourceMapPath: string, map: UnsafeSourceMap, projectRoot: string
path.dirname(sourceMapPath),
s.replace(/webpack:\/\/\/\.\//, `${projectRoot}/`)
)
return absoluteSourcePath.replace(projectRoot, '').replace(/^\//, '')
return absoluteSourcePath.replace(projectRoot, '').replace(/^(\/|\\)/, '')
})
}
}
4 changes: 2 additions & 2 deletions src/uploaders/NodeUploader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ export async function uploadOne ({
type: PayloadType.Node,
apiKey,
appVersion,
minifiedUrl: bundle,
minifiedUrl: bundle.replace(/\\/g, '/'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure whether this change needs to be applied to browser & react native uploaders too — it makes sense to me that it does, but all of our unit tests pass on Windows without changing them (the node tests failed until this change). Paths used for browser/react native code would already have to use / to match any files, as the browser matches on URLs and both iOS & Android use unix paths

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy for the same logic to be applied, even in the knowledge that it will be a no-op most of the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into this further, RN has no equivalent minfiedUrl so I don't think any paths need to change. The browser uploader already uses / in the minifiedUrl (because that's what glob does) and doesn't call path.resolve so the uploaded source maps already have the correct URL on Windows

minifiedFile: new File(fullBundlePath, bundleContent),
sourceMap: new File(fullSourceMapPath, JSON.stringify(transformedSourceMap)),
overwrite: overwrite
Expand Down Expand Up @@ -223,7 +223,7 @@ export async function uploadMultiple ({
type: PayloadType.Node,
apiKey,
appVersion,
minifiedUrl: path.relative(projectRoot, path.resolve(absoluteSearchPath, bundlePath)),
minifiedUrl: path.relative(projectRoot, path.resolve(absoluteSearchPath, bundlePath)).replace(/\\/g, '/'),
minifiedFile: (bundleContent && fullBundlePath) ? new File(fullBundlePath, bundleContent) : undefined,
sourceMap: new File(fullSourceMapPath, JSON.stringify(transformedSourceMap)),
overwrite: overwrite
Expand Down