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

tharakawj · 2017-09-17T09:38:15Z

This attempts to implement the idea proposed in #3120.

In this implementation, the script(iframeScript.js) that needs to be executed in the context of the iframe is bundled in a separate webpack build step. Then this script is imported as a text using webpack raw-loader in the main overlay script(index.js) and inject to the iframe with a script tag. After the injected script is executed, it starts to communicate with the main script via some global hooks and continue to update overlay until there are no errors to show.

Based on #2961, #1944, using inline webpack raw-loader might not be a good idea. Any suggestions on that?

Timer · 2017-09-17T13:19:06Z

packages/react-error-overlay/package.json

+    "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"


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?

See #3142 (comment)

Timer · 2017-09-17T13:22:45Z

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

 import { applyStyles } from './utils/dom/css';

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


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

Timer · 2017-09-17T13:25:12Z

packages/react-error-overlay/webpack.config.js

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


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

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.

react-scripts-dangerous · 2017-09-17T13:47:22Z

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

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

npm i [email protected]
# or
yarn add [email protected]
# or
create-react-app [email protected] 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-17T14:12:24Z

packages/react-error-overlay/package.json

@@ -54,6 +56,7 @@
    "flow-bin": "^0.54.0",
    "jest": "20.0.4",
    "jest-fetch-mock": "1.2.1",
+    "raw-loader": "^0.5.1",


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.

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 ...

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

gaearon · 2017-09-17T15:03:49Z

Can we just bundle the entire thing ahead of time? Then we don't need hacks for raw loader because we can use webpack directly.

tharakawj · 2017-09-17T18:14:21Z

@gaearon @Timer I actually tried few ways to bundle entire thing using webpack and came across some issues. That's why I decided to go with this though it's confusing.

First, I tried to bundle everything into one single file with a config like follows but still importing iframeScript.js with the raw loader.

{ 
  entry: './src/index.js',
  output: {
    path: path.join(__dirname, './lib'),
    filename: 'index.js',
    library: 'ReactErrorOverlay',
    libraryTarget: 'umd',
  }
}

The issue with this approach is that since we directly importing iframeScript.js with the raw loader it doesn't bundle and resolve depedencies of iframeScript.js and just import the file as a string.

Then I tried to bundle index.js and iframeScript.js as two separate bundles with two entry points.

{
  entry: { bundle:'./src/iframeScript.js', index: './src/index.js'},
  output: {
    path: path.join(__dirname, './lib'),
    filename: '[name].js',
    library: 'ReactErrorOverlay',
    libraryTarget: 'umd',
  }
}

This gives an error as webpack can't resolve bundle.js in ./src. If I use iframeScript instead of bundle, it will import src/iframeScript.js as a string like in the first approach.

What am I missing here?

Timer · 2017-09-18T12:34:27Z

Creating two separate bundles sounds appropriate; can't you just adjust the resolver to fallback to the lib directory?

Or, maybe simpler, add an alias for iframeScript in the index.js webpack configuration:

alias: {
  iframeScript$: path.resolve(__dirname, '../lib/iframe-bundle.js')
}

and switch import/require to iframeScript instead of ./iframeScript.js.

Sorry if I'm missing the point here.

tharakawj · 2017-09-21T11:14:15Z

This might work. I'll give a try. Thanks!

tharakawj · 2017-09-22T15:13:05Z

Resolving with alias doesn't work in the first run and starts to work perfectly from the second without rimraf lib/. It seems webpack haven't written the ./lib/iframe-bundle.js to the disk by the time it tries to resolve the file for the import in ./src/index.js file. But from the second run, it can resolve ./lib/iframe-bundle.js with the file written in the previous run.

Also, I tried to use multiple configs instead of multiple entry points but it doesn't make any difference.

I have few more ideas to try out. Meanwhile, appreciate if anyone can shed some light on this.

Timer · 2017-09-22T15:14:57Z

You may just need to run webpack twice.

tharakawj · 2017-09-22T16:00:40Z

You mean using a build script with webpack API?

Timer · 2017-09-22T16:04:00Z

I was thinking:

cross-env NODE_ENV=production webpack --config webpack.config.iframe.js && cross-env NODE_ENV=production webpack --config webpack.config.whatever.js

Timer · 2017-09-22T16:05:06Z

You could do a build script with the webpack API, too.

tharakawj · 2017-09-22T16:17:07Z

Yes. I think using a build script will be clearer. I'll write one.

react-scripts-dangerous · 2017-09-24T08:51:42Z

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

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

npm i [email protected]
# or
yarn add [email protected]
# or
create-react-app [email protected] 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-24T18:28:47Z

packages/react-error-overlay/package.json

    "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"


I'm assuming this should be a dev dependency.

Oh, how did I miss that 🤔

Timer · 2017-09-24T18:28:59Z

packages/react-error-overlay/package.json

@@ -35,15 +35,20 @@
    "babel-code-frame": "6.22.0",
    "babel-runtime": "6.26.0",
    "html-entities": "1.2.1",
+    "object-assign": "^4.1.1",
+    "promise": "^8.0.1",


Let's pin these versions.

react-scripts-dangerous · 2017-09-25T01:12:01Z

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

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

npm i [email protected]
# or
yarn add [email protected]
# or
create-react-app [email protected] 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

LGTM. Feel free to merge [via squash] if you think this is ready.

Timer · 2017-09-29T02:51:14Z

packages/react-error-overlay/webpack.config.iframe.js

+          {
+            loader: 'babel-loader',
+            options: {
+              cacheDirectory: true,


Let's not cache just to be safe.

Timer · 2017-09-29T02:51:20Z

packages/react-error-overlay/webpack.config.js

+          {
+            loader: 'babel-loader',
+            options: {
+              cacheDirectory: true,


tharakawj · 2017-09-30T00:43:21Z

Ok. I'll merge this later today.

react-scripts-dangerous · 2017-10-03T02:02:31Z

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

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

npm i [email protected]
# or
yarn add [email protected]
# or
create-react-app [email protected] 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-03T16:58:44Z

…react-app * 'master' of https://github.com/facebookincubator/create-react-app: Make error overlay to run in the context of the iframe (facebook#3142) Fix Windows compatibility (facebook#3232) Fix package management link in README (facebook#3227) Watch for changes in `src/**/node_modules` (facebook#3230) More spec compliant HTML template (facebook#2914) Minor change to highlight dev proxy behaviour (facebook#3075) Correct manual proxy documentation (facebook#3185) Improve grammar in README (facebook#3211) Publish Fix license comments Changelog for 1.0.14 BSD+Patents -> MIT (facebook#3189) Add link to active CSS modules discussion (facebook#3163) Update webpack-dev-server to 2.8.2 (facebook#3157) Part of class fields to stage 3 (facebook#2908) Update unclear wording in webpack config docs (facebook#3160) Display pid in already running message (facebook#3131) Link local react-error-overlay package in kitchensink test

…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) [email protected] 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 17, 2017

tharakawj mentioned this pull request Sep 17, 2017

Completely isolate the error overlay script from the main app #3120

Closed

tharakawj force-pushed the isolate-error-overlay branch from bbab2b8 to 63eab15 Compare September 17, 2017 09:54

Timer reviewed Sep 17, 2017

View reviewed changes

Timer reviewed Sep 24, 2017

View reviewed changes

Timer approved these changes Sep 29, 2017

View reviewed changes

tharakawj force-pushed the isolate-error-overlay branch from 009fc01 to bec0436 Compare October 3, 2017 00:58

tharakawj added 7 commits October 3, 2017 20:34

Make error overlay to run in the context of the iframe

92a79f4

Configure webpack to build the entire package

0a2acf5

Remove inline raw-loder config

0b48c89

Configure watch mode for error-overlay webpack build

f569f45

Add pollyfills to the error-overlay iframe script

a52592f

Add header comment

483e631

Configure to fail CI on error or warning

fd248f7

tharakawj added 4 commits October 3, 2017 20:34

Suppress flowtype error on importing iframe-bundle

7796f54

Change webpack to a dev dependency and pin some versions

272c491

Disable webpack cache

4dff32a

Update license headers to MIT

57274a4

tharakawj force-pushed the isolate-error-overlay branch from bec0436 to 57274a4 Compare October 3, 2017 15:05

tharakawj merged commit cd3d04b into facebook:master Oct 3, 2017

tharakawj added the tag: internal label Oct 3, 2017

Timer added this to the 1.0.15 milestone Oct 3, 2017

Timer mentioned this pull request Oct 4, 2017

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

Merged

tharakawj mentioned this pull request Oct 11, 2017

react-error-overlay has no dependencies now (it's bundled) #3263

Merged

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

This was referenced Feb 2, 2018

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

Open

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

Closed

dav-is mentioned this pull request Sep 25, 2018

[WIP] Strong CSP Support vercel/next.js#4943

Closed

14 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tharakawj commented Sep 17, 2017 •

edited

Loading

Timer Sep 17, 2017 •

edited

Loading

tharakawj Sep 17, 2017

Timer Sep 17, 2017

Timer Sep 17, 2017

tharakawj Sep 17, 2017

react-scripts-dangerous commented Sep 17, 2017

Timer Sep 17, 2017

Timer Sep 17, 2017

tharakawj Sep 17, 2017

gaearon commented Sep 17, 2017

tharakawj commented Sep 17, 2017

Timer commented Sep 18, 2017 •

edited

Loading

tharakawj commented Sep 21, 2017

tharakawj commented Sep 22, 2017

Timer commented Sep 22, 2017

tharakawj commented Sep 22, 2017

Timer commented Sep 22, 2017

Timer commented Sep 22, 2017

tharakawj commented Sep 22, 2017

react-scripts-dangerous commented Sep 24, 2017

Timer Sep 24, 2017

tharakawj Sep 24, 2017

Timer Sep 24, 2017

react-scripts-dangerous commented Sep 25, 2017

Timer left a comment

Timer Sep 29, 2017

Timer Sep 29, 2017

tharakawj commented Sep 30, 2017

react-scripts-dangerous commented Oct 3, 2017

Timer commented Oct 3, 2017

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

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

Conversation

tharakawj commented Sep 17, 2017 • edited Loading

Timer Sep 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

react-scripts-dangerous commented Sep 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Sep 17, 2017

tharakawj commented Sep 17, 2017

Timer commented Sep 18, 2017 • edited Loading

tharakawj commented Sep 21, 2017

tharakawj commented Sep 22, 2017

Timer commented Sep 22, 2017

tharakawj commented Sep 22, 2017

Timer commented Sep 22, 2017

Timer commented Sep 22, 2017

tharakawj commented Sep 22, 2017

react-scripts-dangerous commented Sep 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

react-scripts-dangerous commented Sep 25, 2017

Timer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tharakawj commented Sep 30, 2017

react-scripts-dangerous commented Oct 3, 2017

Timer commented Oct 3, 2017

tharakawj commented Sep 17, 2017 •

edited

Loading

Timer Sep 17, 2017 •

edited

Loading

Timer commented Sep 18, 2017 •

edited

Loading