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

feat: Isolate iframe code from parent window #57

Merged
merged 5 commits into from Apr 8, 2019

Conversation

@markdalgleish
Copy link
Member

commented Apr 5, 2019

Each iframe is now a separate document (frame.html), as opposed to our previous strategy where code was executed in the parent window and then rendered into blank iframes (via react-frame-component).

This is actually a massive re-architecture in how code is evaluated and executed, which solves a number of issues:

  • Component code is now executed within the frame, so any inspection of the browser window or browser globals will now work as expected (e.g. fixes #45).
  • Hot reloading of CSS now works, since webpack-dev-server is now inserting styles within each frame, rather than adding them to the parent document.
  • CSS no longer needs to be injected into the iframe manually on page load. The iframes now own their own styles as any page typically would.

It turned out that this model is inherently slower, so I've also included the following performance improvements that seem to make up the difference:

  • The error boundary wrapping each frame no longer causes a remount whenever code changes, allowing React to properly optimise each re-render when new code is provided.
  • The source JSX is now only compiled once, whereas previously we were compiling it once to validate it, then once per iframe.
  • The debounce for updating code is now 500ms instead of 200ms, greatly decreasing the odds of re-rendering locking up the main thread.
  • Since we now have two separate HTML files being generated, I've enabled chunking to ensure that the output is better optimised to be shared between them.

Breaking Changes

  • FrameComponent is longer provided with the frameWindow and width props. These are redundant now that you can just access them directly as you normally would, via window and window.innerWidth.

Development notes

I needed to update to the latest alpha release of html-webpack-plugin so that it could handle our multi-page chunking strategy correctly.

@mattsjones
Copy link

left a comment

Looks pretty good to me. A couple of suggested changes but nothing really blocking.

optimization: {
nodeEnv: options.production ? 'production' : 'development',
minimize: options.production,
concatenateModules: options.production,

This comment has been minimized.

Copy link
@mattsjones

mattsjones Apr 8, 2019

The top 3 options are actually the defaults so can probably drop unless you want to be explicit.

children: PropTypes.node.isRequired
};

state = {
error: null,
erroneousCode: null,

This comment has been minimized.

Copy link
@mattsjones

mattsjones Apr 8, 2019

I think invalidCode would be easier to type lol 🤷‍♂️


const getQueryParams = (url = window.location.href) => {
try {
const hash = url.split('#')[1] || '';

This comment has been minimized.

Copy link
@mattsjones

mattsjones Apr 8, 2019

Can we just use window.location.hash instead?

};
}

componentDidMount() {}

This comment has been minimized.

Copy link
@mattsjones

mattsjones Apr 8, 2019

Remove?

constructor(props) {
super(props);

this.state = {

This comment has been minimized.

Copy link
@mattsjones

mattsjones Apr 8, 2019

Needs hooks 😉

This comment has been minimized.

Copy link
@markdalgleish

markdalgleish Apr 8, 2019

Author Member

Yep, gonna do a sweep of this at some point.

@jackhurley23
Copy link
Contributor

left a comment

👌

@markdalgleish markdalgleish merged commit 03e41a3 into master Apr 8, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@markdalgleish markdalgleish deleted the isolate-iframes branch Apr 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.