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

Projects
None yet
7 participants
@tharakawj
Copy link
Contributor

commented Jun 11, 2017

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 tharakawj:reactify-error-overlay branch 3 times, most recently from a97e090 to 59e421a Jun 11, 2017

@gaearon

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2017

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 tharakawj:reactify-error-overlay branch from 59e421a to fd7c996 Jul 1, 2017

@tharakawj tharakawj force-pushed the tharakawj:reactify-error-overlay branch 2 times, most recently from c1af487 to 5736b21 Jul 9, 2017

@tharakawj

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2017

@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

This comment has been minimized.

Copy link
Member

commented Jul 9, 2017

Retrying. Fingers crossed!

@gaearon

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2017

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

tharakawj added some commits Jun 11, 2017

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.

@tharakawj tharakawj force-pushed the tharakawj:reactify-error-overlay branch from 5736b21 to 822a5f6 Jul 15, 2017

@tharakawj

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2017

🎉 @gaearon ready for the review.

@cr101

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2017

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

@gaearon

This comment has been minimized.

Copy link
Member

commented Jul 18, 2017

It is already possible. Although API will change.

gaearon added some commits Aug 26, 2017

@gaearon

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2017

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

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

if (typeof onSourceClick === 'function') {

This comment has been minimized.

Copy link
@gaearon

gaearon Aug 27, 2017

Member

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

This comment has been minimized.

Copy link
@tharakawj

tharakawj Aug 27, 2017

Author Contributor

Fixed!

}

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

This comment has been minimized.

Copy link
@gaearon

gaearon Aug 27, 2017

Member

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 gaearon referenced this pull request Aug 28, 2017

Merged

Changelog for 1.0.12 #3016

@gaearon

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2017

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2017

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

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2017

Yeah would love to get this into Gatsby!

@gaearon

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented Aug 28, 2017

Can you give me an example to look at?

@jaredpalmer

This comment has been minimized.

@gaearon

This comment has been minimized.

Copy link
Member

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

matar
Merge branch 'master' of https://github.com/facebookincubator/create-…
…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 added a commit to figitaki/create-react-app that referenced this pull request Aug 31, 2017

Convert react-error-overlay to React (facebook#2515)
* 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

Changelog for 1.0.13
## 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 react-scripts@1.0.13
```

or

```
yarn add --exact react-scripts@1.0.13
```

## 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 react-scripts@1.0.12
```

or

```
yarn add --exact react-scripts@1.0.12
```

**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 (#2515)
* 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

Ayc0 pushed a commit to Ayc0/create-react-app that referenced this pull request Sep 18, 2017

Convert react-error-overlay to React (facebook#2515)
* 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

someden pushed a commit to someden/create-react-app that referenced this pull request Sep 20, 2017

Convert react-error-overlay to React (facebook#2515)
* 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 added a commit to thongdong7/create-react-app that referenced this pull request Sep 24, 2017

Convert react-error-overlay to React (facebook#2515)
* 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 added a commit to thongdong7/create-react-app that referenced this pull request Sep 24, 2017

Convert react-error-overlay to React (facebook#2515)
* 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 added a commit to kasperpeulen/create-react-app that referenced this pull request Sep 24, 2017

Convert react-error-overlay to React (facebook#2515)
* 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 (#2515)
* 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

jdcrensh added a commit to jdcrensh/create-react-app that referenced this pull request Oct 24, 2017

Convert react-error-overlay to React (facebook#2515)
* 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.
You can’t perform that action at this time.