Add click-to-open support for build errors #3100

tharakawj · 2017-09-10T01:48:44Z

Resolves #2805

This a basic implementation of "click to open in editor" feature for build(compile time) errors and We can improve it based on the feedback coming from the community.

Currently, it only supports Babel syntax errors and ESLint errors. What are the other common or known error types that we can consider here?

Also, please note that this includes a refactoring to the programming interface of the error overlay and it will break any project that uses the package separately without CRA.

Timer · 2017-09-10T04:04:29Z

packages/react-dev-utils/webpackHotDevClient.js

@@ -25,6 +25,18 @@ var launchEditorEndpoint = require('./launchEditorEndpoint');
 var formatWebpackMessages = require('./formatWebpackMessages');
 var ErrorOverlay = require('react-error-overlay');

+ErrorOverlay.listenToOpenInEditor(function OpenInEditor({


Why a separate function?

Can we just put this back under ErrorOverlay.startReportingRuntimeErrors under the prop launchEditor?

ErrorOverlay.startReportingRuntimeErrors and its parameters are specific to runtime errors. But listenToOpenInEditor will trigger the listener for both runtime and build errors. So isn't that less intuitive to put OpenInEditor under ErrorOverlay.startReportingRuntimeErrors?

Ah, this makes sense. Thanks!

Timer · 2017-09-10T04:05:08Z

packages/react-error-overlay/src/containers/StackFrame.js

    } = this.props.frame;
-    // Keep this in sync with react-error-overlay/middleware.js


This comment was lost when the code was moved.
I assume it might need updating, or is it not relevant anymore?

Added an updated comment.

Timer · 2017-09-10T16:10:11Z

I wonder why Travis CI is failing. 🤔

TryingToImprove · 2017-09-10T16:15:56Z

Uncaught [TypeError: ErrorOverlay.listenToOpenInEditor is not a function]

Timer · 2017-09-10T16:20:30Z

@TryingToImprove yes, but the function is present and it works on AppVeyor.

I'm interested in why it's failing on Travis specifically, e.g. due to an npm bug, etc.

tharakawj · 2017-09-11T02:50:33Z

I experience the same for kitchensink in my local development environment. Looking further I found it uses bundle.js with old react-error-overlay/index.js. Probably your #3102 will fix this.

Timer · 2017-09-11T03:40:13Z

Can you try it with my PR? Feel free to merge it if it resolves it -- I'm in bed. 😄

tharakawj · 2017-09-11T06:53:42Z

I'll try it in the evening.

tharakawj · 2017-09-11T16:50:47Z

Rebased with the latest master. No idea why it's still failing. 😕

viankakrisna · 2017-09-11T17:41:02Z

@tharakawj i think it's npm 5 bug...

viankakrisna · 2017-09-11T17:43:06Z

see #3105

tharakawj · 2017-09-11T18:36:15Z

I see. I'll rebase and try after #3107

Timer · 2017-09-12T01:40:46Z

#3107 has been merged.

Timer · 2017-09-13T02:29:49Z

Yikes -- looks like CI is still failing. Do you have time to look into this or maybe I could find some time to?

tharakawj · 2017-09-13T02:44:45Z

Yeah! I won't be able to look into this until the weekend. Please have a look if possible.

tharakawj · 2017-09-16T09:00:52Z

Finally, I think I found the actual reason why CI is failing and it will be fixed with #3139. But I'm still curious how it doesn't affect to AppVeyor build.

tharakawj · 2017-09-16T18:04:33Z

Ready to review! 😎

react-scripts-dangerous · 2017-09-16T22:10:55Z

Hello! I'm a bot that helps facilitate testing pull requests.

Your pull request (commit eba9f53caac2452d8c91687784a66a757fcaf07f) has been released on npm for testing purposes.

npm i react-scripts-dangerous@1.0.14-eba9f53.0
# or
yarn add react-scripts-dangerous@1.0.14-eba9f53.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.14-eba9f53.0 folder/

Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk!

Thanks for your contribution!

Timer · 2017-09-17T13:04:07Z

packages/react-dev-utils/webpackHotDevClient.js

@@ -25,6 +25,16 @@ var launchEditorEndpoint = require('./launchEditorEndpoint');
 var formatWebpackMessages = require('./formatWebpackMessages');
 var ErrorOverlay = require('react-error-overlay');

+ErrorOverlay.listenToOpenInEditor(function OpenInEditor(errorLocation) {


IMO, listenToOpenInEditor is a little ambiguous. e.g. can you listen with more than one function, or is this action reversible?
Questions people might have in mind if they're thinking of an EventEmitter (listeners/observers).

What about setEditorCallback or setEditorHandler?

I see your point. Just I couldn't think of any better name than this. Let's go with setEditorHandler.

Timer · 2017-09-17T13:05:20Z

packages/react-error-overlay/src/index.js

 let currentBuildError: null | string = null;
 let currentRuntimeErrorRecords: Array<ErrorRecord> = [];
 let currentRuntimeErrorOptions: null | RuntimeReportingOptions = null;
 let stopListeningToRuntimeErrors: null | (() => void) = null;

+export function listenToOpenInEditor(listener: OpenInEditorListener) {


If this is renamed to setEditorCallback/setEditorHandler, we could make this accept OpenInEditorListener | null so it can be switched off by sending null.

Timer · 2017-09-17T13:10:13Z

packages/react-error-overlay/src/containers/StackFrame.js

@@ -168,7 +157,7 @@ class StackFrame extends Component<Props, State> {
      }
    }

-    const canOpenInEditor = this.getEndpointUrl() !== null;
+    const canOpenInEditor = this.getErrorLocation() !== null;


Shouldn't this also still consider if overlay has a callback set? We don't want to style the text as clickable if it's a noop (if my understanding is correct here).

Can we do away with the openInEditorListener and openInEditor indirection and simply pass what the user has sent all the way through to the component so we can check if it's null?

I'd imagine we could just call update() when the callback is set to ensure props are updated in components.

Yes, I think we should. I'll fix this.

Timer · 2017-09-17T13:15:09Z

packages/react-error-overlay/src/index.js

+}
+
+function openInEditor(errorLoc: ErrorLocation) {
+  if (typeof openInEditorListener === 'function') {


This is the indirection I'm referencing below. Alternatively, we could add a warning, e.g. "You attempted to open a file in your editor but no handler was set".

Timer · 2017-10-04T12:25:41Z

I'd like to release this PR in conjunction with #3142 so that we don't have to bump the major version of react-error-overlay twice.

…runtime error overlays

- Remove indirection of openInEditorListener - Check editorHandler for null before styling error clickable

tharakawj · 2017-10-04T18:33:05Z

@Timer Updated the PR. Here I had check errLoc twice to avoid a flow error. Any better way to do it? Otherwise, if everything is ok, you can merge this and cut the release.

react-scripts-dangerous · 2017-10-04T20:51:43Z

Hello! I'm a bot that helps facilitate testing pull requests.

Your pull request (commit ae18e55) has been released on npm for testing purposes.

npm i react-scripts-dangerous@1.0.15-ae18e55.0
# or
yarn add react-scripts-dangerous@1.0.15-ae18e55.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.15-ae18e55.0 folder/

Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk!

Thanks for your contribution!

Timer · 2017-10-04T21:15:47Z

Oh, I hope I didn't come off as wanting to release ASAP, I just meant to have it in before the next release.

I'll take a look at this soon. 😄

Timer · 2017-10-05T00:43:19Z

What happens if you change const canOpenInEditor = errLoc !== null && editorHandler !== null; to const canOpenInEditor = errLoc != null && editorHandler != null;?

Since errLoc is ?ErrorLocation it may be null or undefined, so you only checked if it was null.

Alternatively, we can make parseCompileError return ErrorLocation | null.
We don't want a maybe type here (?ErrorLocation).

tharakawj · 2017-10-05T02:24:22Z

Unfortunately, neither of these approaches seem to work. Apparently, flow can't refine the type when we check null/undefined through another variable. Try this snippet if you want to play around it.

Timer · 2017-10-05T02:33:23Z

Interesting, it seems flow cannot flow follow those cases; not a problem! I have nothing against the double check.

Timer · 2017-10-06T00:22:16Z

…react-app * 'master' of https://github.com/facebookincubator/create-react-app: Update README.md Fix dead link to Jest "expect" docs (facebook#3289) Use production React version for bundled overlay (facebook#3267) Add warning when using `react-error-overlay` in production (facebook#3264) Add external links to deployment services (facebook#3265) `react-error-overlay` has no dependencies now (facebook#3263) Add click-to-open support for build errors (facebook#3100) Update style-loader and disable inclusion of its HMR code in builds (facebook#3236) Update url-loader to 0.6.2 for mime ReDoS vuln (facebook#3246)

…pescript * 'master' of https://github.com/wmonk/create-react-app-typescript: (265 commits) fix typo in changelog Update README For 2.13.0 v2.13.0 Remove tslint-loader from prod build (again) Include TypeScript as devDependency in boilerplate output Documented how to define custom module formats for the TypeScript compiler so that you can import images and other files (references wmonk#172) v2.12.0 Update README For 2.12.0 Update typescript to 2.6.2 v2.11.0 Update changelog for 2.11.0 Fixed problem with tsconfig.json baseUrl and paths Update createJestConfig.js Update changelog for 2.10.0 v2.10.0 Readd transformIgnorePatterns Update react-dev-utils Update package.json dependencies Readd Missing raf Package Update JestConfig Creation Fix Fix Missing Variable Fix package.json Merge pull request wmonk#204 from StefanSchoof/patch-1 Merge pull request wmonk#201 from StefanSchoof/patch-1 Merge pull request wmonk#199 from DorianGrey/master Merge pull request wmonk#165 from johnnyreilly/master Publish Add 1.0.17 changelog (#3402) Use new WebpackDevServer option (#3401) Fix grammar in README (#3394) Add link to VS Code troubleshooting guide (#3399) Update VS Code debug configuration (#3400) Update README.md (#3392) Publish Reorder publishing instructions Changelog for 1.0.16 (#3376) Update favicon description (#3374) Changelog for 1.0.15 (#3357) Replace template literal; fixes #3367 (#3368) CLI@1.4.2 Publish Add preflight CWD check for npm (#3355) Stop using `npm link` in tests (#3345) Fix for add .gitattributes file #3080 (#3122) Mention that start_url needs to be "." for client side routing start using npm-run-all to build scss and js (#2957) Updating the Service Worker opt-out documentation (#3108) Remove an useless negation in registerServiceWorker.js (#3150) Remove output.path from dev webpack config (#3158) Add `.mjs` support (#3239) Add documentation for Enzyme 3 integration (#3286) Make uglify work in Safari 10.0 - fixes #3280 (#3281) Fix favicon sizes value in manifest (#3287) Bump dependencies (#3342) recommend react-snap; react-snapshot isn't upgraded for React 16 (#3328) Update appveyor.cleanup-cache.txt Polyfill rAF in test environment (#3340) Use React 16 in development Use a simpler string replacement for the overlay Clarify the npm precompilation advice --no-edit Update `eslint-plugin-react` (#3146) Add jest coverage configuration docs (#3279) Update link to Jest Expect docs (#3303) Update README.md Fix dead link to Jest "expect" docs (#3289) v2.8.0 Use production React version for bundled overlay (#3267) Add warning when using `react-error-overlay` in production (#3264) Add external links to deployment services (#3265) `react-error-overlay` has no dependencies now (#3263) Add click-to-open support for build errors (#3100) Update style-loader and disable inclusion of its HMR code in builds (#3236) Update url-loader to 0.6.2 for mime ReDoS vuln (#3246) Make error overlay to run in the context of the iframe (#3142) Upgrade to typescript 2.5.3 Fix Windows compatibility (#3232) Fix package management link in README (#3227) Watch for changes in `src/**/node_modules` (#3230) More spec compliant HTML template (#2914) Minor change to highlight dev proxy behaviour (#3075) Correct manual proxy documentation (#3185) Improve grammar in README (#3211) Publish Fix license comments Changelog for 1.0.14 BSD+Patents -> MIT (#3189) Add link to active CSS modules discussion (#3163) Update webpack-dev-server to 2.8.2 (#3157) Part of class fields to stage 3 (#2908) Update unclear wording in webpack config docs (#3160) Display pid in already running message (#3131) Link local react-error-overlay package in kitchensink test Resolved issue #2971 (#2989) Revert "run npm 5.4.0 in CI (#3026)" (#3107) Updated react-error-overlay to latest Flow (0.54.0) (#3065) Auto-detect running editor on Linux for error overlay (#3077) Clean target directory before compiling overlay (#3102) Rerun prettier and pin version (#3058) ...

facebook-github-bot added the CLA Signed label Sep 10, 2017

Timer reviewed Sep 10, 2017

View reviewed changes

tharakawj force-pushed the build-error-open-editor branch from 5765617 to 8fe1724 Compare September 11, 2017 16:13

viankakrisna mentioned this pull request Sep 11, 2017

Revert "run npm 5.4.0 in CI (#3026)" #3107

Merged

tharakawj force-pushed the build-error-open-editor branch from 8fe1724 to 9064ca9 Compare September 12, 2017 16:53

tharakawj mentioned this pull request Sep 16, 2017

Link local react-error-overlay package in kitchensink test #3139

Merged

tharakawj force-pushed the build-error-open-editor branch from 9064ca9 to eba9f53 Compare September 16, 2017 15:50

Timer reviewed Sep 17, 2017

View reviewed changes

Timer added the tag: enhancement label Sep 29, 2017

Timer added this to the 1.0.15 milestone Sep 29, 2017

tharakawj added 3 commits October 4, 2017 21:48

Implement click-to-open for babel syntax errors in build error overlay

830c4e9

Add click-to-open support for lint errors and refactor parser

51236e9

Reactor code to reuse open-in-editor functionality in both build and …

de8e9b6

…runtime error overlays

tharakawj added 5 commits October 4, 2017 21:55

Fix some eslint warnings

5c0cccf

Add a comment about keeping middleware and dev client in sync

88cd256

Remove es6 features from webpack dev client

64ef981

Make open-in-editor functionality to work with new iframe script

8d36cd9

Rename openInEditor to editorHandler

a0b3566

- Remove indirection of openInEditorListener - Check editorHandler for null before styling error clickable

tharakawj force-pushed the build-error-open-editor branch from eba9f53 to a0b3566 Compare October 4, 2017 17:48

Fix flow errors

ae18e55

Timer merged commit 00ed100 into facebook:master Oct 6, 2017

gaearon mentioned this pull request Oct 30, 2017

Changelog for 1.0.15 #3357

Merged

dependencies bot mentioned this pull request Oct 31, 2017

7 packages updated by dependencies.io storybookjs/storybook#2200

Merged

tharakawj mentioned this pull request Dec 11, 2017

Jump to editor from build time overlay #2805

Closed

This was referenced Feb 2, 2018

fix(deps): update dependency react-scripts to v1.1.5 - autoclosed SaraVieira/react-payment-request#17

Closed

Update dependency react-scripts to v1.1.1 - autoclosed kkostov/react-calendar-icon#32

Closed

lock bot locked and limited conversation to collaborators Jan 20, 2019

		} = this.props.frame;
		// Keep this in sync with react-error-overlay/middleware.js

Add click-to-open support for build errors #3100

Add click-to-open support for build errors #3100

Conversation

tharakawj commented Sep 10, 2017 • edited Loading

Timer Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

tharakawj Sep 10, 2017

Choose a reason for hiding this comment

Timer Sep 10, 2017

Choose a reason for hiding this comment

Timer Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

tharakawj Sep 10, 2017

Choose a reason for hiding this comment

Timer commented Sep 10, 2017

TryingToImprove commented Sep 10, 2017

Timer commented Sep 10, 2017

tharakawj commented Sep 11, 2017

Timer commented Sep 11, 2017

tharakawj commented Sep 11, 2017

tharakawj commented Sep 11, 2017

viankakrisna commented Sep 11, 2017

viankakrisna commented Sep 11, 2017

tharakawj commented Sep 11, 2017 • edited Loading

Timer commented Sep 12, 2017

Timer commented Sep 13, 2017

tharakawj commented Sep 13, 2017

tharakawj commented Sep 16, 2017

tharakawj commented Sep 16, 2017

react-scripts-dangerous commented Sep 16, 2017

Timer Sep 17, 2017

Choose a reason for hiding this comment

tharakawj Sep 21, 2017

Choose a reason for hiding this comment

Timer Sep 17, 2017 • edited Loading

Choose a reason for hiding this comment

Timer Sep 17, 2017 • edited Loading

Choose a reason for hiding this comment

tharakawj Sep 21, 2017

Choose a reason for hiding this comment

Timer Sep 17, 2017 • edited Loading

Choose a reason for hiding this comment

Timer commented Oct 4, 2017

tharakawj commented Oct 4, 2017

react-scripts-dangerous commented Oct 4, 2017

Timer commented Oct 4, 2017

Timer commented Oct 5, 2017 • edited Loading

tharakawj commented Oct 5, 2017

Timer commented Oct 5, 2017

Timer commented Oct 6, 2017

tharakawj commented Sep 10, 2017 •

edited

Loading

Timer Sep 10, 2017 •

edited

Loading

Timer Sep 10, 2017 •

edited

Loading

tharakawj commented Sep 11, 2017 •

edited

Loading

Timer Sep 17, 2017 •

edited

Loading

Timer Sep 17, 2017 •

edited

Loading

Timer Sep 17, 2017 •

edited

Loading

Timer commented Oct 5, 2017 •

edited

Loading