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.

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

Convert react-error-overlay to React #2515

Merged
merged 15 commits into from
Aug 28, 2017

Conversation

tharakawj
Copy link
Contributor

Proposed in #2100

Some points to discuss,

  1. CodeBlock component still depends on old DOM manipulation code and dangerouslySetInnerHTML. Any suggestions to make it better?
  2. Used HTML button elements instead of clickable (tab clickable) divs with similar styling. Any downside of that?
  3. I'm not much sure about the purpose of consumeEvent.js. Can you think of a better alternative to get the same behavior with React?

@tharakawj tharakawj force-pushed the reactify-error-overlay branch 3 times, most recently from a97e090 to 59e421a Compare June 11, 2017 07:07
@gaearon
Copy link
Contributor

gaearon commented Jun 11, 2017

Wow, thanks for tackling this! I'm a bit busy now but will review next time I get the chance.

@gaearon
Copy link
Contributor

gaearon commented Jun 27, 2017

Hey, sorry for long time no review. I'm putting out some fires related to Node 8 and Windows but hoping to get back to this soon. In the meantime, do you want to try converting the compile-time overlay to this set of components as well? That was the original plan—right now there's too much duplication between them.

@tharakawj
Copy link
Contributor Author

No problem. Yes. I was thinking to change the compile-time overlay as well. But later I thought to wait until I get feedback for this. Anyway, if this looks good I'll try that and update the PR.

@tharakawj tharakawj force-pushed the reactify-error-overlay branch from 59e421a to fd7c996 Compare July 1, 2017 01:12
@tharakawj tharakawj force-pushed the reactify-error-overlay branch 2 times, most recently from c1af487 to 5736b21 Compare July 9, 2017 06:16
@tharakawj
Copy link
Contributor Author

@gaearon Updated PR with changes to compile-time overlay. I had to refactor several files and that's why more files have changed. Let me know if you need it to be a separate PR or anything to make it easier to review.

I'm not sure why AppVeyor build has failed. Maybe we need to trigger the build manually again.

@gaearon
Copy link
Contributor

gaearon commented Jul 9, 2017

Retrying. Fingers crossed!

@gaearon
Copy link
Contributor

gaearon commented Jul 12, 2017

Looks like Flow is choking:

\node_modules/react-dev-utils/node_modules/react-error-overlay/node_modules/react-dev-utils/node_modules/react-error-overlay/node_modules/eslint-plugin-jsx-a11y/src/util/hasAccessibleChild.js:5
  5: import isHiddenFromScreenReader from './isHiddenFromScreenReader';
                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ./isHiddenFromScreenReader. Required module not found

This is because we use a version of jsx-a11y plugin that happened to publish its internal Flow sources on npm. If you explicitly add it to [ignore] section in .flowconfig this should be fixed.

@tharakawj
Copy link
Contributor Author

@gaearon Thanks for the tip. I'll update and see.

 * Refactor react-error-overlay components to container and presentational components.

 * Make the compile-time error overlay a part of react-error-overlay package.

 * Use react-error-overlay as dependency in react-dev-utils to show compile-time errors.
@tharakawj tharakawj force-pushed the reactify-error-overlay branch from 5736b21 to 822a5f6 Compare July 15, 2017 08:10
@tharakawj
Copy link
Contributor Author

🎉 @gaearon ready for the review.

@cr101
Copy link
Contributor

cr101 commented Jul 17, 2017

@tharakawj Would it be possible to easily integrate react-error-overlay in a non-CRA project?

@gaearon
Copy link
Contributor

gaearon commented Jul 18, 2017

It is already possible. Although API will change.

@gaearon
Copy link
Contributor

gaearon commented Aug 26, 2017

I finally got the time to look into this. I think this is mostly good except for some minor things. I wanted to check—would you be willing to finish this up in response to review, or would you have somebody take over this? I understand it's been a long time and you might be busy or uninterested. In any case thanks for much for this!

@tharakawj
Copy link
Contributor Author

@gaearon Yes! I'm here. Let's finish this up.

frameLink.appendChild(frameAnchor);
frame.appendChild(frameLink);

if (typeof onSourceClick === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this part got lost. I can't click on the source code snippet but I used to be able to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

}

function showCompileErrorOverlay(error: string) {
if (container == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we check iframeReference instead? What if we call showCompileErrorOverlay() twice in a row before mountOverlayIframe callback returns? Wouldn't that result in two iframes being created?

@gaearon
Copy link
Contributor

gaearon commented Aug 28, 2017

I'll just get it in. #yolo

@gaearon gaearon merged commit ecd1f05 into facebook:master Aug 28, 2017
@gaearon gaearon mentioned this pull request Aug 28, 2017
@gaearon
Copy link
Contributor

gaearon commented Aug 28, 2017

Aaand it's out in 1.0.12! Please give it a try. Thanks again :-)
I'll write up some next work directions if you're interested.

@jaredpalmer
Copy link
Contributor

Aside from the path to the bundle being at static/js/bundle.js, what other assumptions does error overlay make about a React app? Want to use it outside of CRA.

@gaearon
Copy link
Contributor

gaearon commented Aug 28, 2017

I don't think it does any. (And we probably want to get rid of that bundle.js assumption too. Need to pass it from outside with the options.)

@tharakawj
Copy link
Contributor Author

@gaearon Nice! Yes please, I'm happy to contribute more.

@KyleAMathews
Copy link
Contributor

Yeah would love to get this into Gatsby!

@gaearon
Copy link
Contributor

gaearon commented Aug 28, 2017

Happy to take a PR if there's something that doesn't let it work out of the box for you.

@jaredpalmer
Copy link
Contributor

jaredpalmer commented Aug 28, 2017

So I have a slightly modified version of the latest webpackHotDevClient that makes the socket connection on another port (3001 instead of window.location.port) because that's where webpack-dev-server runs in Razzle. Compile / lint errors work, but can't seem to get the runtime error overlay working...

Thoughts?

Build Errors ✅

razzle-error-overlay

Runtime Errors ❌

kapture 2017-08-28 at 12 17 24

@gaearon
Copy link
Contributor

gaearon commented Aug 28, 2017

Can you give me an example to look at?

@jaredpalmer
Copy link
Contributor

@gaearon
Copy link
Contributor

gaearon commented Aug 28, 2017

The event.error property is null so we can't extract that data. This happens when the code that throws is from a different origin than the app.

screen shot 2017-08-28 at 1 49 18 pm

To fix it I added a crossorigin attribute to the bundle <script> tag:

-        <script src="${assets.client.js}" defer></script>
+        <script src="${assets.client.js}" crossorigin defer></script>

That fixed it. jaredpalmer/react-error-overlay-razzle-bug#1

screen shot 2017-08-28 at 1 56 59 pm

matart15 pushed a commit to matart15/create-react-app that referenced this pull request Aug 29, 2017
…react-app

* 'master' of https://github.com/facebookincubator/create-react-app: (36 commits)
  Publish
  Changelog for 1.0.12 (facebook#3016)
  Relax React dep requirements
  Default Favicon lossless optimisation (facebook#2917)
  update babel-runtime dependency in react-error-overlay and react-scripts (facebook#2991)
  Convert react-error-overlay to React (facebook#2515)
  Bump react-dev-utils
  Fix module function name in error overlay (facebook#3012)
  Docs: debugging in WebStorm (facebook#2986)
  Fix docs for `printFileSizesAfterBuild` (facebook#2942)
  Remove Modulus from user guide (facebook#2948)
  Remove superfluous lodash usage (facebook#2938)
  Update README.md (facebook#2927)
  Publish
  Prepare for 1.0.11 release (facebook#2924)
  Update dev deps (facebook#2923)
  Update README.md
  Use env variable to disable source maps (facebook#2818)
  Make formatWebpackMessages return all messages (facebook#2834)
  Adjust the `checkIfOnline` check if in a corporate proxy environment (facebook#2884)
  ...
figitaki pushed a commit to figitaki/create-react-app that referenced this pull request Aug 31, 2017
* Convert react-error-overlay to React

* Update compile-time error overlay to use react-error-overlay components

 * Refactor react-error-overlay components to container and presentational components.

 * Make the compile-time error overlay a part of react-error-overlay package.

 * Use react-error-overlay as dependency in react-dev-utils to show compile-time errors.

* Run Prettier

* Move the function name fix into StackFrame itself

* Fix clicking on source code snippet to open the code in editor

* Use exact objects + minor style tweak

* Don't linkify frames that don't exist on the disk

* Fix lint

* Consolidate iframe rendering logic

* Remove circular dependency between react-dev-utils and react-error-overlay

* Fix lint

* Fix decoupling of react-dev-utils and react-error-overlay by moving middleware

* Deduplicate identical errors
gaearon added a commit that referenced this pull request Sep 2, 2017
## 1.0.13 (September 2, 2017)

#### 🐛 Bug Fix

* `react-error-overlay`

  * [#3051](#3051) Fix case-sensitivity issue with upgrading the package version. ([@tharakawj](https://github.com/tharakawj))

* `react-dev-utils`

  * [#3049](#3049) Print filesize difference for chunks. ([@esturcke](https://github.com/esturcke))

* `react-scripts`

  * [#3046](#3046) Fix crash in development mode on IE11. ([@tharakawj](https://github.com/tharakawj))

#### 💅 Enhancement

* `react-scripts`

  * [#3033](#3033) Add an empty mock for `child_process` to let some libraries compile. ([@McFlurriez](https://github.com/McFlurriez))

#### 🏠 Internal

* `react-dev-utils`, `react-error-overlay`

  * [#3028](#3028) Make error overlay filename configurable. ([@jaredpalmer](https://github.com/jaredpalmer))

#### Committers: 4

- Anthony ([McFlurriez](https://github.com/McFlurriez))
- Erik J. Sturcke ([esturcke](https://github.com/esturcke))
- Jared Palmer ([jaredpalmer](https://github.com/jaredpalmer))
- Tharaka Wijebandara ([tharakawj](https://github.com/tharakawj))

### Migrating from 1.0.12 to 1.0.13

Inside any created project that has not been ejected, run:

```
npm install --save --save-exact [email protected]
```

or

```
yarn add --exact [email protected]
```

## 1.0.12 (August 28, 2017)

#### 🐛 Bug Fix

* `react-error-overlay`
  * [#3012](#3012) Fix module function name in error overlay. ([@gaearon](https://github.com/gaearon))

* `react-dev-utils`
  * [#2938](#2938) Remove superfluous lodash usage. ([@Timer](https://github.com/Timer))

#### 💅 Enhancement

* `react-scripts`

  * [#2917](#2917) Optimize the size of default favicon. ([@sylvainbaronnet](https://github.com/sylvainbaronnet))

#### 📝 Documentation

* `react-scripts`

  * [#2986](#2986) Docs: debugging in WebStorm. ([@prigara](https://github.com/prigara))
  * [#2948](#2948) Remove Modulus from user guide. ([@Zertz](https://github.com/Zertz))
  * [#2927](#2927) Update README.md. ([@tbassetto](https://github.com/tbassetto))

* `react-dev-utils`

  * [#2942](#2942) Fix docs for `printFileSizesAfterBuild`. ([@Kerumen](https://github.com/Kerumen))

#### 🏠 Internal

* `react-error-overlay`, `react-scripts`

  * [#2991](#2991) Update `babel-runtime` dependency ([@christophehurpeau](https://github.com/christophehurpeau))

* `react-dev-utils`, `react-error-overlay`, `react-scripts`

  * [#2515](#2515) Convert `react-error-overlay` to React ([@tharakawj](https://github.com/tharakawj))

#### Committers: 9

- Christophe Hurpeau ([christophehurpeau](https://github.com/christophehurpeau))
- Dan Abramov ([gaearon](https://github.com/gaearon))
- Ekaterina Prigara ([prigara](https://github.com/prigara))
- Joe Haddad ([Timer](https://github.com/Timer))
- Pier-Luc Gendreau ([Zertz](https://github.com/Zertz))
- Sylvain Baronnet ([sylvainbaronnet](https://github.com/sylvainbaronnet))
- Tharaka Wijebandara ([tharakawj](https://github.com/tharakawj))
- Thomas Bassetto ([tbassetto](https://github.com/tbassetto))
- Yann Pringault ([Kerumen](https://github.com/Kerumen))

### Migrating from 1.0.11 to 1.0.12

Inside any created project that has not been ejected, run:

```
npm install --save --save-exact [email protected]
```

or

```
yarn add --exact [email protected]
```

**Note:** there’s a [known issue](#3041) that might cause the project to not compile after upgrading. In this case, migrate straight to `1.0.13` which doesn’t have this issue.
JohnNilsson referenced this pull request in JohnNilsson/create-react-app-typescript Sep 9, 2017
* Convert react-error-overlay to React

* Update compile-time error overlay to use react-error-overlay components

 * Refactor react-error-overlay components to container and presentational components.

 * Make the compile-time error overlay a part of react-error-overlay package.

 * Use react-error-overlay as dependency in react-dev-utils to show compile-time errors.

* Run Prettier

* Move the function name fix into StackFrame itself

* Fix clicking on source code snippet to open the code in editor

* Use exact objects + minor style tweak

* Don't linkify frames that don't exist on the disk

* Fix lint

* Consolidate iframe rendering logic

* Remove circular dependency between react-dev-utils and react-error-overlay

* Fix lint

* Fix decoupling of react-dev-utils and react-error-overlay by moving middleware

* Deduplicate identical errors
thongdong7 pushed a commit to thongdong7/create-react-app that referenced this pull request Sep 24, 2017
* Convert react-error-overlay to React

* Update compile-time error overlay to use react-error-overlay components

 * Refactor react-error-overlay components to container and presentational components.

 * Make the compile-time error overlay a part of react-error-overlay package.

 * Use react-error-overlay as dependency in react-dev-utils to show compile-time errors.

* Run Prettier

* Move the function name fix into StackFrame itself

* Fix clicking on source code snippet to open the code in editor

* Use exact objects + minor style tweak

* Don't linkify frames that don't exist on the disk

* Fix lint

* Consolidate iframe rendering logic

* Remove circular dependency between react-dev-utils and react-error-overlay

* Fix lint

* Fix decoupling of react-dev-utils and react-error-overlay by moving middleware

* Deduplicate identical errors
thongdong7 pushed a commit to thongdong7/create-react-app that referenced this pull request Sep 24, 2017
* Convert react-error-overlay to React

* Update compile-time error overlay to use react-error-overlay components

 * Refactor react-error-overlay components to container and presentational components.

 * Make the compile-time error overlay a part of react-error-overlay package.

 * Use react-error-overlay as dependency in react-dev-utils to show compile-time errors.

* Run Prettier

* Move the function name fix into StackFrame itself

* Fix clicking on source code snippet to open the code in editor

* Use exact objects + minor style tweak

* Don't linkify frames that don't exist on the disk

* Fix lint

* Consolidate iframe rendering logic

* Remove circular dependency between react-dev-utils and react-error-overlay

* Fix lint

* Fix decoupling of react-dev-utils and react-error-overlay by moving middleware

* Deduplicate identical errors
kasperpeulen pushed a commit to kasperpeulen/create-react-app that referenced this pull request Sep 24, 2017
* Convert react-error-overlay to React

* Update compile-time error overlay to use react-error-overlay components

 * Refactor react-error-overlay components to container and presentational components.

 * Make the compile-time error overlay a part of react-error-overlay package.

 * Use react-error-overlay as dependency in react-dev-utils to show compile-time errors.

* Run Prettier

* Move the function name fix into StackFrame itself

* Fix clicking on source code snippet to open the code in editor

* Use exact objects + minor style tweak

* Don't linkify frames that don't exist on the disk

* Fix lint

* Consolidate iframe rendering logic

* Remove circular dependency between react-dev-utils and react-error-overlay

* Fix lint

* Fix decoupling of react-dev-utils and react-error-overlay by moving middleware

* Deduplicate identical errors
swengorschewski referenced this pull request in swengorschewski/cra-typescript-electron Oct 16, 2017
* Convert react-error-overlay to React

* Update compile-time error overlay to use react-error-overlay components

 * Refactor react-error-overlay components to container and presentational components.

 * Make the compile-time error overlay a part of react-error-overlay package.

 * Use react-error-overlay as dependency in react-dev-utils to show compile-time errors.

* Run Prettier

* Move the function name fix into StackFrame itself

* Fix clicking on source code snippet to open the code in editor

* Use exact objects + minor style tweak

* Don't linkify frames that don't exist on the disk

* Fix lint

* Consolidate iframe rendering logic

* Remove circular dependency between react-dev-utils and react-error-overlay

* Fix lint

* Fix decoupling of react-dev-utils and react-error-overlay by moving middleware

* Deduplicate identical errors
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants