Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Switch to unix path separators before normalizing path for Windows compatibility #3079

Merged
merged 4 commits into from
Sep 6, 2017
Merged

Switch to unix path separators before normalizing path for Windows compatibility #3079

merged 4 commits into from
Sep 6, 2017

Conversation

Timer
Copy link
Contributor

@Timer Timer commented Sep 6, 2017 •
edited
Loading

Turns out that the path module stubbed into a web environment does not support Windows, nor contains the posix / win32 variants of path (it's fixed posix, so path.sep returns / in a win32 environment).

Effectively, what happened here is that the fileName was C:\foo\bar and upon normalizing, remained C:\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.

@Timer Timer added this to the 1.0.13 milestone Sep 6, 2017
@Timer Timer mentioned this pull request Sep 6, 2017
Closed
@Timer
Copy link
Contributor Author

Timer commented Sep 6, 2017

Hmm, @gaearon -- isn't the "Check your code at" part a little redundant?

@gaearon
Copy link
Contributor

gaearon commented Sep 6, 2017

Redundant in what way?

gaearon
gaearon reviewed Sep 6, 2017
View reviewed changes
packages/react-error-overlay/src/utils/unmapper.js Outdated
@@ -56,7 +56,7 @@ async function unmap(
}
let { fileName } = frame;
if (fileName) {
fileName = path.normalize(fileName);
fileName = path.normalize(fileName.replace(/[\\]+/g, '/'));
Copy link
Contributor

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.

@Timer
Copy link
Contributor Author

Timer commented Sep 6, 2017

Redundant in what way?

Just that we're displaying the code with contextual highlighting, but still telling the user to check the code at that location.
Other errors (e.g. document.body()) don't add that extra instruction; I'm not sure of a place where it'd be particularly useful.

@gaearon
Copy link
Contributor

gaearon commented Sep 6, 2017

We should probably make it consistent, but this can be fixed in React itself rather than here.

@Timer
Copy link
Contributor Author

Timer commented Sep 6, 2017

Hmm, wouldn't that degrade the experience for those not using react-error-overlay?

@gaearon
Copy link
Contributor

gaearon commented Sep 6, 2017

Degrade how? I assume they'd still see the stack trace. Error overlay just strips it out.

@Timer
Copy link
Contributor Author

Timer commented Sep 6, 2017

Hmm, I suppose it wouldn't then.

@Timer Timer merged commit 634dadb into facebook:master Sep 6, 2017
@Timer Timer deleted the hotfix/3078 branch September 6, 2017 18:42
@gaearon
Copy link
Contributor

gaearon commented Sep 6, 2017

@Timer Do you want to cut a release for this?

@Timer
Copy link
Contributor Author

Timer commented Sep 7, 2017

Sure, if you'd like!
I can cut one in about 10 hours if you don't get to it before then; I'm pretty tied up tonight.

@Timer
Copy link
Contributor Author

Timer commented Sep 7, 2017 •
edited
Loading

Hmm, what's odd is that lerna wants me to update many packages:

Joes-MacBook-Pro:create-react-app joe$ npx lerna updated
lerna info version 2.1.2
lerna info versioning independent
lerna info Checking for updated packages...
lerna info Comparing with [email protected].
lerna info result 
- eslint-config-react-app
- react-dev-utils
- react-error-overlay
- react-scripts

I'd assume it'd only prompt me to do react-error-overlay and eslint-config-react-app (for docs); so I'm not sure why react-dev-utils and react-scripts (except that they're dependencies), but they're specified as caret ranges already ... (is this a lerna bug or intended functionality?)

Should we do a manual release of a patch version on the overlay or a whole react-scripts release cycle?

matart15 pushed a commit to matart15/create-react-app that referenced this pull request Sep 13, 2017
Merge branch 'master' of https://github.com/facebookincubator/create-…
8a152ae 
…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)
thongdong7 pushed a commit to thongdong7/create-react-app that referenced this pull request Sep 24, 2017
@Timer @thongdong7
Unmapper Windows compatibility (facebook#3079)
e8b42e0 
* 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
thongdong7 pushed a commit to thongdong7/create-react-app that referenced this pull request Sep 24, 2017
@Timer @thongdong7
Unmapper Windows compatibility (facebook#3079)
888c541 
* 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
kasperpeulen pushed a commit to kasperpeulen/create-react-app that referenced this pull request Sep 24, 2017
@Timer @kasperpeulen
Unmapper Windows compatibility (facebook#3079)
db58bf4 
* 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
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gaearon gaearon gaearon approved these changes

Assignees
No one assigned
Labels
CLA Signed tag: bug fix
Projects
None yet
Milestone
1.0.13
Development

Successfully merging this pull request may close these issues.

Error overlay not showing contextual information for warnings
3 participants
@Timer @gaearon @facebook-github-bot