Skip to content

Make error overlay to run in the context of the iframe #3142

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

Merged
merged 11 commits into from
Oct 3, 2017
Merged

Make error overlay to run in the context of the iframe #3142

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
92a79f4
Make error overlay to run in the context of the iframe
tharakawj Sep 17, 2017
0a2acf5
Configure webpack to build the entire package
tharakawj Sep 23, 2017
0b48c89
Remove inline raw-loder config
tharakawj Sep 23, 2017
f569f45
Configure watch mode for error-overlay webpack build
tharakawj Sep 23, 2017
a52592f
Add pollyfills to the error-overlay iframe script
tharakawj Sep 23, 2017
483e631
Add header comment
tharakawj Sep 23, 2017
fd248f7
Configure to fail CI on error or warning
tharakawj Sep 24, 2017
7796f54
Suppress flowtype error on importing iframe-bundle
tharakawj Sep 24, 2017
272c491
Change webpack to a dev dependency and pin some versions
tharakawj Sep 25, 2017
4dff32a
Disable webpack cache
tharakawj Oct 3, 2017
57274a4
Update license headers to MIT
tharakawj Oct 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Make error overlay to run in the context of the iframe
  • Loading branch information
@tharakawj
tharakawj committed Oct 3, 2017
commit 92a79f4203588dc97724526362d11b55ceb14e40
13 changes: 8 additions & 5 deletions packages/react-error-overlay/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
"main": "lib/index.js",
"scripts": {
"prepublishOnly": "npm run build:prod && npm test",
"start": "rimraf lib/ && cross-env NODE_ENV=development npm run build -- --watch",
"test": "flow && jest",
"build": "rimraf lib/ && babel src/ -d lib/",
"build:prod": "rimraf lib/ && cross-env NODE_ENV=production babel src/ -d lib/"
"start": "rimraf lib/ && cross-env NODE_ENV=development webpack --config webpack.config.js -w & NODE_ENV=development babel src/ -d lib/ -w",
"test": "flow && cross-env NODE_ENV=test jest",
"build": "rimraf lib/ && cross-env NODE_ENV=development babel src/ -d lib/ && cross-env NODE_ENV=production webpack --config webpack.config.js",
"build:prod": "rimraf lib/ && cross-env NODE_ENV=production babel src/ -d lib/ && cross-env NODE_ENV=production webpack --config webpack.config.js"
Copy link
Contributor

@Timer Timer Sep 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Why are we still compiling with Babel? We should just let webpack handle all of this if we're making the switch to a bundle imo.

Not sure if we want to support legacy use of the package via lib/, /cc @gaearon.

edit: ah, I see -- we still import index which relies on some modules.

Wouldn't it be better to bundle this entire thing up -- the way it works currently is pretty confusing, unless we make the distinction and actually split the bundled code & unbundled into two separate directories; what do you think?

Copy link
Contributor Author

@tharakawj tharakawj Sep 17, 2017

Choose a reason for hiding this comment

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

See #3142 (comment)

},
"repository": "facebookincubator/create-react-app",
"license": "MIT",
Expand All @@ -33,12 +33,14 @@
"dependencies": {
"anser": "1.4.1",
"babel-code-frame": "6.22.0",
"babel-loader": "^7.1.2",
"babel-runtime": "6.26.0",
"html-entities": "1.2.1",
"react": "^15 || ^16",
"react-dom": "^15 || ^16",
"settle-promise": "1.0.0",
"source-map": "0.5.6"
"source-map": "0.5.6",
"webpack": "^3.6.0"
Copy link
Contributor

@Timer Timer Sep 24, 2017

Choose a reason for hiding this comment

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

I'm assuming this should be a dev dependency.

Copy link
Contributor Author

@tharakawj tharakawj Sep 24, 2017

Choose a reason for hiding this comment

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

Oh, how did I miss that 🤔

},
"devDependencies": {
"babel-cli": "6.24.1",
Expand All @@ -54,6 +56,7 @@
"flow-bin": "^0.54.0",
"jest": "20.0.4",
"jest-fetch-mock": "1.2.1",
"raw-loader": "^0.5.1",
Copy link
Contributor

@Timer Timer Sep 17, 2017

Choose a reason for hiding this comment

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

Testing with the release by @react-scripts-dangerous caught this:

Failed to compile.

./node_modules/react-error-overlay-dangerous/lib/index.js
Module not found: Can't resolve 'raw-loader' in '/Users/joe/Desktop/folder2/node_modules/react-error-overlay-dangerous/lib'

I assume this should be a normal dependency.

Copy link
Contributor

@Timer Timer Sep 17, 2017

Choose a reason for hiding this comment

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

I assume our e2e tests didn't test this because we don't run npm with NODE_ENV as production; but can we do that in our tests -- I assume linking makes this really weird?

Is it worth trying to test in our E2E; or we could make @react-scripts-dangerous also run its own smoke tests and comment on PRs? Not sure how often this will be a problem ...

Copy link
Contributor Author

@tharakawj tharakawj Sep 17, 2017

Choose a reason for hiding this comment

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

Good catch with @react-scripts-dangerous 😀
Maybe in e2e, so that we can find errors before creating a PR??

"rimraf": "^2.6.1"
},
"jest": {
Expand Down
57 changes: 57 additions & 0 deletions packages/react-error-overlay/src/iframeScript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
import React from 'react';
import ReactDOM from 'react-dom';
import CompileErrorContainer from './containers/CompileErrorContainer';
import RuntimeErrorContainer from './containers/RuntimeErrorContainer';
import { overlayStyle } from './styles';
import { applyStyles } from './utils/dom/css';

let iframeRoot = null;

function render({
currentBuildError,
currentRuntimeErrorRecords,
dismissRuntimeErrors,
launchEditorEndpoint,
}) {
if (currentBuildError) {
return <CompileErrorContainer error={currentBuildError} />;
}
if (currentRuntimeErrorRecords.length > 0) {
return (
<RuntimeErrorContainer
errorRecords={currentRuntimeErrorRecords}
close={dismissRuntimeErrors}
launchEditorEndpoint={launchEditorEndpoint}
/>
);
}
return null;
}

window.updateContent = function updateContent(errorOverlayProps) {
let renderedElement = render(errorOverlayProps);

if (renderedElement === null) {
ReactDOM.unmountComponentAtNode(iframeRoot);
return false;
}
// Update the overlay
ReactDOM.render(renderedElement, iframeRoot);
return true;
};

document.body.style.margin = '0';
// Keep popup within body boundaries for iOS Safari
document.body.style['max-width'] = '100vw';
iframeRoot = document.createElement('div');
applyStyles(iframeRoot, overlayStyle);
document.body.appendChild(iframeRoot);
window.parent.__REACT_ERROR_OVERLAY_GLOBAL_HOOK__.iframeReady();
87 changes: 37 additions & 50 deletions packages/react-error-overlay/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@
*/

/* @flow */
import React from 'react';
import type { Element } from 'react';
import ReactDOM from 'react-dom';
import CompileErrorContainer from './containers/CompileErrorContainer';
import RuntimeErrorContainer from './containers/RuntimeErrorContainer';
import { listenToRuntimeErrors } from './listenToRuntimeErrors';
import { iframeStyle, overlayStyle } from './styles';
import { iframeStyle } from './styles';
import { applyStyles } from './utils/dom/css';

/* eslint-disable import/no-webpack-loader-syntax */
//$FlowFixMe
import iframeScript from 'raw-loader!./bundle.js';
Copy link
Contributor

@Timer Timer Sep 17, 2017

Choose a reason for hiding this comment

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

This hack is fine for now IMO, as long as we switch it eventually (once we iron out how to handle raw files).

/* eslint-enable import/no-webpack-loader-syntax */

import type { ErrorRecord } from './listenToRuntimeErrors';

type RuntimeReportingOptions = {|
Expand All @@ -25,8 +25,8 @@ type RuntimeReportingOptions = {|

let iframe: null | HTMLIFrameElement = null;
let isLoadingIframe: boolean = false;
var isIframeReady: boolean = false;

let renderedElement: null | Element<any> = null;
let currentBuildError: null | string = null;
let currentRuntimeErrorRecords: Array<ErrorRecord> = [];
let currentRuntimeErrorOptions: null | RuntimeReportingOptions = null;
Expand Down Expand Up @@ -88,15 +88,14 @@ export function stopReportingRuntimeErrors() {
}

function update() {
renderedElement = render();
// Loading iframe can be either sync or async depending on the browser.
if (isLoadingIframe) {
// Iframe is loading.
// First render will happen soon--don't need to do anything.
return;
}
if (iframe) {
// Iframe has already loaded.
if (isIframeReady) {
// Iframe is ready.
// Just update it.
updateIframeContent();
return;
Expand All @@ -108,58 +107,46 @@ function update() {
loadingIframe.onload = function() {
const iframeDocument = loadingIframe.contentDocument;
if (iframeDocument != null && iframeDocument.body != null) {
iframeDocument.body.style.margin = '0';
// Keep popup within body boundaries for iOS Safari
iframeDocument.body.style['max-width'] = '100vw';
const iframeRoot = iframeDocument.createElement('div');
applyStyles(iframeRoot, overlayStyle);
iframeDocument.body.appendChild(iframeRoot);

// Ready! Now we can update the UI.
iframe = loadingIframe;
isLoadingIframe = false;
updateIframeContent();
const script = loadingIframe.contentWindow.document.createElement(
'script'
);
script.type = 'text/javascript';
script.innerHTML = iframeScript;
iframeDocument.body.appendChild(script);
}
};
const appDocument = window.document;
appDocument.body.appendChild(loadingIframe);
}

function render() {
if (currentBuildError) {
return <CompileErrorContainer error={currentBuildError} />;
}
if (currentRuntimeErrorRecords.length > 0) {
if (!currentRuntimeErrorOptions) {
throw new Error('Expected options to be injected.');
}
return (
<RuntimeErrorContainer
errorRecords={currentRuntimeErrorRecords}
close={dismissRuntimeErrors}
launchEditorEndpoint={currentRuntimeErrorOptions.launchEditorEndpoint}
/>
);
function updateIframeContent() {
if (!currentRuntimeErrorOptions) {
throw new Error('Expected options to be injected.');
}
return null;
}

function updateIframeContent() {
if (iframe === null) {
if (!iframe) {
throw new Error('Iframe has not been created yet.');
}
const iframeBody = iframe.contentDocument.body;
if (!iframeBody) {
throw new Error('Expected iframe to have a body.');
}
const iframeRoot = iframeBody.firstChild;
if (renderedElement === null) {
// Destroy iframe and force it to be recreated on next error

const isRendered = iframe.contentWindow.updateContent({
currentBuildError,
currentRuntimeErrorRecords,
dismissRuntimeErrors,
launchEditorEndpoint: currentRuntimeErrorOptions.launchEditorEndpoint,
});

if (!isRendered) {
window.document.body.removeChild(iframe);
ReactDOM.unmountComponentAtNode(iframeRoot);
iframe = null;
return;
isIframeReady = false;
}
// Update the overlay
ReactDOM.render(renderedElement, iframeRoot);
}

window.__REACT_ERROR_OVERLAY_GLOBAL_HOOK__ =
window.__REACT_ERROR_OVERLAY_GLOBAL_HOOK__ || {};
window.__REACT_ERROR_OVERLAY_GLOBAL_HOOK__.iframeReady = function iframeReady() {
isIframeReady = true;
isLoadingIframe = false;
updateIframeContent();
};
27 changes: 27 additions & 0 deletions packages/react-error-overlay/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
var path = require('path');
var webpack = require('webpack');

module.exports = {
devtool: 'cheap-module-source-map',
entry: './src/iframeScript.js',
Copy link
Contributor

@Timer Timer Sep 17, 2017

Choose a reason for hiding this comment

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

Should we bundle polyfills? 🤔
or we could document it and leave it up to the person importing it ...

Copy link
Contributor Author

@tharakawj tharakawj Sep 17, 2017

Choose a reason for hiding this comment

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

Yeah, I think we should. Polyfills on parent context don't seem to have an effect on the iframe context. We need polyfills to run in some browsers. (see #3034). I'll update.

output: {
path: path.join(__dirname, './lib'),
filename: 'bundle.js',
},
module: {
rules: [
{
test: /\.js$/,
include: path.resolve(__dirname, './src'),
use: [
{
loader: 'babel-loader',
options: {
cacheDirectory: true,
Copy link
Contributor

@Timer Timer Sep 29, 2017

Choose a reason for hiding this comment

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

Ditto.

},
},
],
},
],
},
};