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

Framework: Introduce AsyncLoad for asynchronous rendering by code splitting #5356

Closed
wants to merge 10 commits into from

Conversation

Projects
None yet
6 participants
@aduth
Copy link
Member

commented May 12, 2016

Prompted by discussion for more granular code splitting, this pull request seeks to try an approach for rendering components with dependencies loaded on-demand by introducing new a component which facilitates rendering via asynchronous require.

This pull request is a proof-of-concept and should not be merged as-is. The loading experience could be improved to load the contents of the accordion asynchronously by user intent (e.g. hover accordion heading), not whenever the page is loaded. Further, in the case that the bundle is not loaded yet when the user tries to access its content, we should display some placeholder.

Before After
Before After

Note in the screenshot above that the post-editor chunk has decreased from 3.47MB to 3.36MB, a difference of 0.11MB (110kB).

Testing instructions:

Note that the sharing accordion still renders correctly when visiting the post editor.

/cc @mtias

import React, { Component, PropTypes } from 'react';
import omit from 'lodash/omit';

export default class CodeSplitRender extends Component {

This comment has been minimized.

Copy link
@mtias

mtias May 12, 2016

Member

I was thinking of <AsyncLoad> perhaps?

<EditorSharingAccordion
<CodeSplitRender
require={ function( callback ) {
require( [ 'post-editor/editor-sharing/accordion' ], callback );

This comment has been minimized.

Copy link
@mtias

mtias May 12, 2016

Member

My only concern is readability of the tree if we start having loader wrappers everywhere. Nothing jumps out at me immediately as possible improvements, though.

This comment has been minimized.

Copy link
@aduth

aduth May 12, 2016

Author Member

My only concern is readability of the tree if we start having loader wrappers everywhere. Nothing jumps out at me immediately as possible improvements, though.

Yeah, it's an unfortunate consequence of Webpack only being capable of static analysis for the code splitting, since otherwise would offer something a bit more simplified like:

require="post-editor/editor-sharing/accordion"

...and have the component manage the complexities.

This comment has been minimized.

Copy link
@blowery

blowery May 12, 2016

Contributor

Why does this work? As you stated, webpack only does static analysis, but this would imply that is can do dynamic analysis... i think. Usually webpack is using require.ensure to inject split points.. plain requires, even hidden in functions, should be treated as hard deps that need to be in the same chunk (or a parent chunk)..

This comment has been minimized.

Copy link
@aduth

aduth May 16, 2016

Author Member

Usually webpack is using require.ensure to inject split points

It's not entirely obvious, but this is the AMD (not CommonJS) syntax for asynchronously loading modules (note the callback). Both are supported by Webpack in defining a split point. The reason for using AMD is that it provides the loaded modules as arguments to the callback function, whereas CommonJS passes only require, and the CodeSplitRender function is otherwise not capable of determining which modules it should proceed to load (unless we duplicated the modules array as a separate prop).

This comment has been minimized.

Copy link
@blowery

blowery May 16, 2016

Contributor

Ahhhhhhhh... subtle.

@mtias

This comment has been minimized.

Copy link
Member

commented May 12, 2016

Nice to see this.

@mtias mtias added the [Pri] High label May 30, 2016

@mtias mtias force-pushed the try/editor-code-split branch 3 times, most recently from 0058020 to d831f23 Jul 1, 2016

@mtias mtias force-pushed the try/editor-code-split branch from 5cfcf82 to 4a36949 Jul 4, 2016

@mtias mtias self-assigned this Jul 4, 2016

@@ -326,7 +327,10 @@ const EditorDrawer = React.createClass( {
{ site && ! this.hasHardCodedPostTypeSupports( type ) && (
<QueryPostTypes siteId={ site.ID } />
) }
{ this.renderTaxonomies() }
{ config.isEnabled( 'manage/custom-post-types' ) && ! includes( [ 'post', 'page' ], this.props.type )

This comment has been minimized.

Copy link
@aduth

aduth Jul 5, 2016

Author Member

Tab in here. In fact, I think we should drop the ! includes part of the condition altogether, but fine to leave that as a separate task if you'd prefer.

Edit: As in, this.renderTaxonomies should be called for any post type (including post and pages), but renderCategoriesAndTags called for post and page only (above renderTaxonomies I assume)

This comment has been minimized.

Copy link
@mtias

mtias Jul 6, 2016

Member

Makes sense, I haven't moved taxonomies to be async-loaded because it was causing issues. Leaving that up to your or @timmyc's discretion :)

@aduth

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2016

The code-splitting feature flag is indeed a bit of an issue to accommodate as this stands. I don't think we can prevent Webpack from splitting the bundles, even if wrapping it in a condition (because static analysis). I wonder if we'd need another/enhanced server/bundler/loader.js to dynamically change the source depending on the feature flag setting. On the upside, this might let us simplify the props of the component (like noted above). Not sure how feasible or what the implementation of this would look like, but I think we'd want to try to avoid locking ourselves into the component usage vs. "raw" code splitting (require.ensure or AMD require).

@mtias

This comment has been minimized.

Copy link
Member

commented Jul 6, 2016

I wonder if we'd need another/enhanced server/bundler/loader.js to dynamically change the source depending on the feature flag setting.

@ehg any thoughts on this? Also @mcsf as you once looked at cleaning up the bundler loader file.

@ehg

This comment has been minimized.

Copy link
Member

commented Jul 8, 2016

I wonder if we'd need another/enhanced server/bundler/loader.js to dynamically change the source depending on the feature flag setting.

I think we'd need a separate loader. Are you thinking it'd just do something simple like check for require="somedep" and replace it with the AMD syntax?

@aduth

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2016

Are you thinking it'd just do something simple like check for require="somedep" and replace it with the AMD syntax?

Yeah, and we'd need a fallback mechanism for when code splitting is disabled.

@mcsf

This comment has been minimized.

Copy link
Member

commented Jul 9, 2016

Sorry, missed the ping. I'll echo the opinion that a separate loader should do it. This could be an opportunity to resume some cleanup work on loaders (#2028) that I had to set aside in the past.

@mcsf mcsf referenced this pull request Jul 9, 2016

Closed

Framework: Add webpack loader to rewrite AsyncLoad calls #6660

0 of 3 tasks complete
@mcsf

This comment has been minimized.

Copy link
Member

commented Jul 9, 2016

Are you thinking it'd just do something simple like check for require="somedep" and replace it with the AMD syntax?

A working example with require="my-sites/drafts/draft-list" can be seen at #6660. It still lacks the following:

Yeah, and we'd need a fallback mechanism for when code splitting is disabled.

@aduth

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2016

Offline, we were discussing how we could achieve this with improved usability and to avoid AMD syntax using a Babel plugin. Here's a gist to the work in progress:

https://gist.github.com/aduth/e4d2eaf11e04b56077d0ac229e44d7d8

Desired usage is similar to as demonstrated in #6660 , effectively transforming:

<AsyncLoad
    require="./sharing"
    ensure={ this.state.shouldLoad }
    placeholder={ <Spinner /> } />

Into:

<AsyncLoad
    require={ ( callback ) => {
        require.ensure( './sharing', () => {
            callback( require( './sharing' ) );
        } );
    } }
    ensure={ this.state.shouldLoad }
    placeholder={ <Spinner /> } />

Optionally depending on the value of the config feature flag, though we'd discussed whether it might be possible to eliminate this feature flag altogether (including for desktop environments).

The implementation of AsyncLoad would remain almost the same as it is here already, with enhancements to allow on-demand ensure. This would enable the module to only be loaded after the user's interactions indicate an intent to use the component (for example, hovering over the accordion title in the post editor sidebar).

cc @umurkontaci @seear @mcsf

@aduth aduth changed the title Framework: Introduce CodeSplitRender for asynchronous rendering by code splitting Framework: Introduce AsyncLoad for asynchronous rendering by code splitting Oct 4, 2016

@aduth

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2016

Superseded by #8544

@aduth aduth closed this Oct 28, 2016

@aduth aduth deleted the try/editor-code-split branch Oct 28, 2016

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