Skip to content

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

Merged
Timer merged 4 commits into
react:masterfrom
Timer:hotfix/3078
Sep 6, 2017
Merged

Switch to unix path separators before normalizing path for Windows compatibility#3079
Timer merged 4 commits into
react:masterfrom
Timer:hotfix/3078

Conversation

@Timer

@TimerTimer commented Sep 6, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

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.

@TimerTimer added this to the 1.0.13 milestone Sep 6, 2017
@TimerTimer mentioned this pull request Sep 6, 2017
Closed
@Timer

Timer commented Sep 6, 2017

Copy link
Copy Markdown
ContributorAuthor

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

@gaearon

gaearon commented Sep 6, 2017

Copy link
Copy Markdown
Contributor

Redundant in what way?

gaearon
gaearon reviewed Sep 6, 2017
View reviewed changes
Comment threadpackages/react-error-overlay/src/utils/unmapper.js
let { fileName } = frame;
if (fileName) {
fileName = path.normalize(fileName);
fileName = path.normalize(fileName.replace(/[\\]+/g, '/'));

@gaearongaearonSep 6, 2017

Copy link
Copy Markdown
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

Timer commented Sep 6, 2017

Copy link
Copy Markdown
ContributorAuthor

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

gaearon commented Sep 6, 2017

Copy link
Copy Markdown
Contributor

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

@Timer

Timer commented Sep 6, 2017

Copy link
Copy Markdown
ContributorAuthor

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

@gaearon

gaearon commented Sep 6, 2017

Copy link
Copy Markdown
Contributor

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

@Timer

Timer commented Sep 6, 2017

Copy link
Copy Markdown
ContributorAuthor

Hmm, I suppose it wouldn't then.

@Timer
Timer merged commit 634dadb into react:masterSep 6, 2017
@Timer
Timer deleted the hotfix/3078 branch September 6, 2017 18:42
@gaearon

gaearon commented Sep 6, 2017

Copy link
Copy Markdown
Contributor

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

@Timer

Timer commented Sep 7, 2017

Copy link
Copy Markdown
ContributorAuthor

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

Timer commented Sep 7, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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 react-dev-utils@4.0.1.
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
matar
Merge branch 'master' ofhttps://github.com/facebookincubator/create-…
8a152ae
…react-app
* 'master' of https://github.com/facebookincubator/create-react-app:
Resolved issue react#2971 (react#2989)
Revert "run npm 5.4.0 in CI (react#3026)" (react#3107)
Updated react-error-overlay to latest Flow (0.54.0) (react#3065)
Auto-detect running editor on Linux for error overlay (react#3077)
Clean target directory before compiling overlay (react#3102)
Rerun prettier and pin version (react#3058)
Reload the page when an error has occurred (react#3098)
run npm 5.4.0 in CI (react#3026)
Unmapper Windows compatibility (react#3079)
Update eslint-config npm install command (react#3072)
Set travis config to use 'precise' ci environment
Publish
Changelog for 1.0.13
Add missing slash
Make error overlay filename configurable (react#3028)
provide empty mock for child_process so importing libraries with it works (react#3033)
Rename Overlay to ErrorOvelay (react#3051)
Strip hash from chunk file name (react#3049)
Fix error overlay 'Object.assign' issue in IE by including polyfills before webpack client (react#3046)
thongdong7 pushed a commit to thongdong7/create-react-app that referenced this pull request Sep 24, 2017
@Timer@thongdong7
Unmapper Windows compatibility (react#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 (react#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 (react#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
@locklockBot locked and limited conversation to collaborators Jan 20, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@gaearongaearongaearon approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signedtag: 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