-
Notifications
You must be signed in to change notification settings - Fork 9
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
Conversation
@@ -102,7 +102,7 @@ export async function uploadOne ({ | |||
type: PayloadType.Node, | |||
apiKey, | |||
appVersion, | |||
minifiedUrl: bundle, | |||
minifiedUrl: bundle.replace(/\\/g, '/'), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@@ -102,7 +102,7 @@ export async function uploadOne ({ | |||
type: PayloadType.Node, | |||
apiKey, | |||
appVersion, | |||
minifiedUrl: bundle, | |||
minifiedUrl: bundle.replace(/\\/g, '/'), |
There was a problem hiding this comment.
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.
Goal
Uploading source maps from Windows has a couple of issues currently: