-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch to unix path separators before normalizing path for Windows compatibility #3079
Conversation
Hmm, @gaearon -- isn't the "Check your code at" part a little redundant? |
Redundant in what way? |
@@ -56,7 +56,7 @@ async function unmap( | |||
} | |||
let { fileName } = frame; | |||
if (fileName) { | |||
fileName = path.normalize(fileName); | |||
fileName = path.normalize(fileName.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.
Would be nice to add a comment for why it helps.
Just that we're displaying the code with contextual highlighting, but still telling the user to check the code at that location. |
We should probably make it consistent, but this can be fixed in React itself rather than here. |
This reverts commit 742bace.
Hmm, wouldn't that degrade the experience for those not using |
Degrade how? I assume they'd still see the stack trace. Error overlay just strips it out. |
Hmm, I suppose it wouldn't then. |
@Timer Do you want to cut a release for this? |
Sure, if you'd like! |
Hmm, what's odd is that lerna wants me to update many packages:
I'd assume it'd only prompt me to do Should we do a manual release of a patch version on the overlay or a whole |
…react-app * 'master' of https://github.com/facebookincubator/create-react-app: Resolved issue facebook#2971 (facebook#2989) Revert "run npm 5.4.0 in CI (facebook#3026)" (facebook#3107) Updated react-error-overlay to latest Flow (0.54.0) (facebook#3065) Auto-detect running editor on Linux for error overlay (facebook#3077) Clean target directory before compiling overlay (facebook#3102) Rerun prettier and pin version (facebook#3058) Reload the page when an error has occurred (facebook#3098) run npm 5.4.0 in CI (facebook#3026) Unmapper Windows compatibility (facebook#3079) Update eslint-config npm install command (facebook#3072) Set travis config to use 'precise' ci environment Publish Changelog for 1.0.13 Add missing slash Make error overlay filename configurable (facebook#3028) provide empty mock for child_process so importing libraries with it works (facebook#3033) Rename Overlay to ErrorOvelay (facebook#3051) Strip hash from chunk file name (facebook#3049) Fix error overlay 'Object.assign' issue in IE by including polyfills before webpack client (facebook#3046)
* Switch to unix path separators before normalizing path for Windows compatibility * Add comment for posterity * Revert "Add comment for posterity" This reverts commit 742bace. * Strictly add comment
* Switch to unix path separators before normalizing path for Windows compatibility * Add comment for posterity * Revert "Add comment for posterity" This reverts commit 742bace. * Strictly add comment
* Switch to unix path separators before normalizing path for Windows compatibility * Add comment for posterity * Revert "Add comment for posterity" This reverts commit 742bace. * Strictly add comment
Turns out that the
path
module stubbed into a web environment does not support Windows, nor contains theposix
/win32
variants of path (it's fixedposix
, sopath.sep
returns/
in a win32 environment).Effectively, what happened here is that the
fileName
wasC:\foo\bar
and upon normalizing, remainedC:\foo\bar
(it should've switched to/
, or more specifically,path.sep
).This aligns the behavior with the other path normalization process as seen here: https://github.com/facebookincubator/create-react-app/blob/b17fa4123e2d098d943f4b33ae9c5c2ac311fab6/packages/react-error-overlay/src/utils/unmapper.js#L67
Fixes #3078.