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

Lazily load CodeMirror assets #4500

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Jan 16, 2018

This PR should have its base set to master once #4348 is merged!

🤠 What this is

In #4348, we added the CodeMirror library so as to provide syntax highlighting and a nice editor experience for writing HTML blocks.

Unfortunately, though, this means an extra ~1.9 MB of assets are loaded when initialising Gutenberg.

The solution is to lazily load these assets. That is, rather than loading the CodeMirror library when Gutenberg initialises, we load it when a HTML block is first inserted into the post.

This PR accomplishes this by changing the CodeEditor component to use a new withLazyDependencies HOC:

export default withLazyDependencies( {
scripts() {
const scripts = [
'wp-codemirror',
'code-editor',
'htmlhint',
'csslint',
'jshint',
];
// Don't load htmlhint-kses unless we need it
if ( window._wpGutenbergCodeEditorSettings.htmlhint.kses ) {
scripts.push( 'htmlhint-kses' );
}
return scripts;
},
styles: [ 'wp-codemirror', 'code-editor' ],
} )( CodeEditor );

When CodeEditor is mounted, a <script src="/wp-admin/wp-load-scripts.php?load..."> node and a <link rel="stylesheet" href="/wp-admin/wp-load-styles.php?load=..."> node is inserted into the DOM which causes the assets to be asynchronously loaded.

Check out the included README to read more about withLazyDependencies .

✅ How to test

  1. Insert a HTML block
  2. Type some code into it. You should see syntax highlighting
  3. Save the post and reload the page. The HTML block with syntax highlighting should remain

@noisysocks noisysocks added the [Status] In Progress Tracking issues with work in progress label Jan 16, 2018
@gziolo
Copy link
Member

gziolo commented Jan 16, 2018

https://webpack.js.org/guides/lazy-loading/ - I guess this would be a regular way to async load assets, but those seem to be very special so I think it this might be the best way to proceed. Keep working on it 👍

import withLazyDependencies from '../higher-order/with-lazy-dependencies';

// TODO: How can we avoid repeating all of this?
const defaultSettings = {
Copy link
Member

Choose a reason for hiding this comment

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

How is it exposed for Plugins' editor?

Copy link
Member Author

@noisysocks noisysocks Jan 17, 2018

Choose a reason for hiding this comment

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

Normally these settings are assigned into wp.codeEditor.defaultSettings by wp_enqueue_code_editor here:

https://github.com/WordPress/wordpress-develop/blob/8f95800d52c1736d651ae6e259f90ad4a0db2c3f/src/wp-includes/general-template.php#L3576

But since the whole idea of this PR is to avoid calling wp_enqueue_code_editor (since it enqueues a lot of heavy assets), I've had to re-declare these editor settings.

One possibility is that we could have Core expose the default settings separately e.g. with a new wp_default_code_editor_settings() method.

Any better ideas, @westonruter?

Copy link
Member

Choose a reason for hiding this comment

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

@noisysocks We actually did have a separate function originally for getting the settings, but then combined them into wp_enqueue_code_editor(): WordPress/better-code-editing@70b96ca

Oh the irony.

Until a wp_default_code_editor_settings() is introduced, a workaround could be to do this:

$gutenberg_editor_settings = array();
function gutenberg_capture_code_editor_settings_and_short_circuit_enqueues( $settings ) {
    global $gutenberg_editor_settings;
    $gutenberg_editor_settings = $settings;
    return false;
}
add_filter( 'wp_code_editor_settings', 'gutenberg_capture_code_editor_settings_and_short_circuit_enqueues', 1000 );
wp_enqueue_code_editor();
remove_filter( 'wp_code_editor_settings', 'gutenberg_capture_code_editor_settings_and_short_circuit_enqueues', 1000 );

Copy link
Member Author

@noisysocks noisysocks Jan 17, 2018

Choose a reason for hiding this comment

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

That's a fun hack!

Thanks Weston. I'll play with it, and (eventually) create a trac ticket to put back wp_code_editor_settings.

@@ -62,8 +140,19 @@ class CodeEditor extends Component {
}

render() {
return <textarea ref={ ref => this.textarea = ref } value={ this.props.value } />;
return <textarea ref={ ref => ( this.textarea = ref ) } value={ this.props.value } />;
Copy link
Member

Choose a reason for hiding this comment

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

In other places I often see something like this:

// constructor
this.bindTextarea = ref => ( this.textarea = ref );

// render
return <textarea ref={ this.bindTextarea } value={ this.props.value } />;

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, isn't this pattern preferred because it means bindTextarea doesn't change from render-to-render and so it is slightly more performant?

componentDidMount() {
loadDependencies( scripts, styles ).then( () => {
this.setState( { hasLoaded: true } );
} );
Copy link
Member

Choose a reason for hiding this comment

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

What if any of the dependencies won't load?

Copy link
Member Author

@noisysocks noisysocks Jan 17, 2018

Choose a reason for hiding this comment

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

Aye, we'll need to add a loading state and error state to withLazyDependencies.

Maybe something like this:

const LazyComponent = withLazyDependencies( {
    scripts: [ 'foo', 'bar' ],
    styles: [ 'baz' ],
    loadingComponent() {
        return <Spinner />;
    },
    errorComponent( message ) {
        return <div>{ sprintf( __( 'Uh oh! %s' ), message ) }</div>
    },
} )( MyComponent );

@noisysocks
Copy link
Member Author

https://webpack.js.org/guides/lazy-loading/ - I guess this would be a regular way to async load assets, but those seem to be very special so I think it this might be the best way to proceed.

In this PR we lazily load the assets that are regularly added to the page with wp_enqueue_script or wp_enqueue_style.

But, yes, this is different to what we usually mean by lazy loading which is having Webpack defer the loading of a particular chunk until the application actually needs it.

I've tried to avoid this ambiguity by choosing to use the word dependency. This is what we call enqueued assets and styles in Core. I'm open to other suggestions though! 😀

@noisysocks noisysocks force-pushed the add/syntax-highlighting-to-html-block branch from e2c619d to 7714d1e Compare January 18, 2018 03:53
Introduces a new <CodeEditor> component which wraps a CodeMirror
enhanced <textarea>. This component is then used to provide syntax
highliting in the Custom HTML block.
This prevents the cursor from moving around to different blocks while
editing code.
Make the CodeEditor in the HTML block look vaguely like what Joen mocked
up in #1386.
Prevent the iframe from capturing pointer events and preventing the user
from being able to select the block.
`componentDidUpdate` and `componentDidMount` fire after DOM
reconcilation but before the frame is painted. CodeMirror's `focus()`
method seems to only work *after* paint. Scheduling `focus()` to happen
after the next frame is painted seems to be the best that we can do.
We only care that the component renders without an error and doesn't
change unintentionally, which makes this a perfect use case for snapshot
testing.
Bumps up the z-index of every element that is at the same depth as the
block toolbar. By making these indicies greater than 4, we ensure our
contextual elements appear over the CodeMirror editor. This stops the
CodeMirror gutter from appearing over the block toolbar.
- Permit pressing ESCAPE to unfocus the currently selected block.
- Allow pressing UP/DOWN to move focus to the previous/next block when
  the cursor is at the start/end of the editor.
@noisysocks noisysocks force-pushed the add/syntax-highlighting-to-html-block branch from 7714d1e to 7847ea2 Compare January 18, 2018 04:14
@noisysocks noisysocks force-pushed the add/lazy-loading-to-codemirror branch 2 times, most recently from e438832 to b1db31d Compare January 18, 2018 05:29
@@ -102,4 +183,29 @@ class CodeEditor extends Component {
}
}

export default CodeEditor;
export default withLazyDependencies( {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about calling it withLazyLoadedAssets?

Copy link
Member Author

@noisysocks noisysocks Jan 22, 2018

Choose a reason for hiding this comment

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

I like dependency since it's the word we use in Core. Happy to be persuaded otherwise though 🙂

'csslint',
'jshint',
// TODO: Gotta check if user can unfiltered_html
...( true ? [ 'htmlhint-kses' ] : [] ),
Copy link
Member

Choose a reason for hiding this comment

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

You can always give an option to pass object or function to this HOC. When there is a function you can call it with all props passed to the component and based on this perform required filtering.

const alreadyLoaded = {};

function loadScript( url ) {
if ( alreadyLoaded[ url ] ) {
Copy link
Member

Choose a reason for hiding this comment

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

We perform url based checks but pass a list of registered assets. What if there are two concurrent component being loaded at the same time where the same asset is being requested? Example:

withLazyDependencies( { scripts: [ 'foo', 'bar' ] } ( ComponentA );
withLazyDependencies( { scripts: [ 'foo', 'dummy' ] } ( ComponentB );

Should we care? I guess it depends on what we load, if scripts have side-effects then we probably should avoid loading it twice.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to solve it now? Not necessarily. I'm sharing so we were ready to take that into account when more than one component uses this HOC.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! We should change this to check handles rather than URLs.

@noisysocks noisysocks force-pushed the add/lazy-loading-to-codemirror branch from b1db31d to 3bb0874 Compare January 19, 2018 05:05
@aduth
Copy link
Member

aduth commented Jan 20, 2018

I'm glad that there was effort put into making this generic with the withLazyDependencies higher-order component. I still think there's a bit more we could do here (e.g. does each block need to define its own placeholder? maybe at least have a default?), and that defining the script dependencies is redundant if we consider that a block should be defining its script enqueue handle via the newly-introduced editor_script property (#4039). Assuming these scripts are defined via wp_register_script, we should already know what the dependencies are (third argument of the function). The complication for us is that we don't have separate script enqueues for each individual core block. But... maybe we should 😄

There was a bit of mention of such an idea in #2756:

Once it's determined that a block's browser-specific implementation is used (either in content, by insertion, or common / recent blocks) the script could then be loaded asynchronously.

This could be further optimized to determine server-side by user preferences or post content what block scripts should be loaded with the page synchronously (i.e. available at page load).

@noisysocks
Copy link
Member Author

does each block need to define its own placeholder? maybe at least have a default?

I'll change the HOC to define a default loading/error placeholder 🙂

Assuming these scripts are defined via wp_register_script, we should already know what the dependencies are (third argument of the function). The complication for us is that we don't have separate script enqueues for each individual core block. But... maybe we should 😄

This is a cool idea. It makes a lot of sense to implement asynchronous editor_script loading and change the HTML block to use server-side block registration.

Since the withLazyDependencies HOC would make implementing asynchronous editor_script loading more straightforward, I think the best path forward is to treat this PR as a good first step towards the vision you've outlined here.

@noisysocks noisysocks force-pushed the add/lazy-loading-to-codemirror branch from 3bb0874 to 811ed1a Compare January 22, 2018 03:49
Adds a higher order function which wraps a component and lazily loads
registered scripts and styles. This lets us avoid using
wp_enqueue_code_editor() which adds considerable bulk (~ 1.9 MB) to the
page load.
Avoid having to re-declare our CodeMirror configuration by capturing the
settings from a wp_enqueue_code_editor() filter.
`htmlhint-kses` is only needed for users who don't have the
`unfiltered_html` capability. We can determine this by checking for the
presense of `wp.codeEditor.defaultSettingshtmlhint.kses`.
@noisysocks noisysocks force-pushed the add/lazy-loading-to-codemirror branch from 811ed1a to 87714ec Compare January 22, 2018 04:53
@noisysocks noisysocks changed the title [WIP] Lazily load CodeMirror assets Lazily load CodeMirror assets Jan 22, 2018
@noisysocks noisysocks added [Feature] Blocks Overall functionality of blocks [Type] Performance Related to performance efforts and removed [Status] In Progress Tracking issues with work in progress labels Jan 22, 2018
@noisysocks
Copy link
Member Author

This is ready to be reviewed now 😊

@dmsnell
Copy link
Member

dmsnell commented Jan 22, 2018

This feels a bit odd to me for a couple reasons though I love the direction it's going.

First of all it's odd to me that we're communicating via globals here instead of what feels more idiomatic to JavaScript projects. That is, if I had a lazy-loading wrapper component I'd expect to pass it a function with takes the dependencies and produces a component.

withLazyStuff( {
  scripts: [ 'CodeMirror' ]
} )( ( { CodeMirror } ) => ( {
  edit( attributes ) {  }
  save(  ) { … }
} )

And inherent in that would be some indication that each dependency is either loading or loaded. Instead here it seems like we're waiting for all the dependencies to load and then we completely bail without drawing the wrapped component if any dependency fails.

That's the second thing that I question here. Why should we block the render of a component just because it's not fully available? (Please pardon me if I'm misunderstanding the wrapper). When I put together the CodeMirror-based code editor, for example, I wanted to provide a basic <textarea> until the CodeMirror loaded, which is accomplished via System.import() or just import() if we've updated webpack since then.

I'd love to encourage a system of progressive enhancement where the wrapped component still has the control over its own render and lifecycle so that we don't give each other the all-or-nothing proposition.

Finally, and maybe I think you wrote the right code here, but there's an inherent tradeoff to be made between using import() and loading an enqueue_script call to WordPress. Did you look into native JavaScript code bundling and loading as an alternative?

Thanks for working on this! It's super important.

@noisysocks
Copy link
Member Author

noisysocks commented Jan 23, 2018

First of all it's odd to me that we're communicating via globals here instead of what feels more idiomatic to JavaScript projects.
Did you look into native JavaScript code bundling and loading as an alternative?

The core assumption that I've been working off of is that we want to use the version of CodeMirror that is exposed via wp_enqueue_code_editor() (as of WordPress 4.9). The benefit of using this is that we get all of the CodeMirror configuration that WordPress sets up for free.

WordPress exposes this configured editor via the wp.codeEditor global, which is why withLazyDependencies does not behave in the more functional way that you describe.

I didn't look into using using import() to include our own codemirror package, since doing so would mean that we would have to duplicate most of that configuration that WordPress gives us. It would also mean that the version of CodeMirror that we bundle in Gutenberg could differ from the version we bundle in WordPress.

I'd love to encourage a system of progressive enhancement where the wrapped component still has the control over its own render and lifecycle so that we don't give each other the all-or-nothing proposition.
I wanted to provide a basic <textarea> until the CodeMirror loaded

This is a cool idea. I worry though that the UX might be jarring. For example, if a user begins typing code into the plain text area, we would have to be careful to not lose their place once CodeMirror is loaded. I'm not totally convinced that the benefits of this kind of progressive enhancement outweigh the additional complexity involved in implementing it.

@dmsnell
Copy link
Member

dmsnell commented Jan 23, 2018

The core assumption that I've been working off of is that we want to use the version of CodeMirror that is exposed via wp_enqueue_code_editor()

that seems like a very good reason for a clear comment on why we're diverging from what I think many people will be expecting. // there's a reason for this global implicit coupling, don't "fix it"

I didn't look into using using import() to include our own codemirror package, since doing so would mean that we would have to duplicate most of that configuration that WordPress gives us. It would also mean that the version of CodeMirror that we bundle in Gutenberg could differ from the version we bundle in WordPress.

I'm not sure this is our tradeoff. We may still be able to ensure they are the same. I don't think it has to be solved here in order to resolve #4500 but at the same time I feel much more hesitant to build this as a generalized system in the wrapper vs. just performing this behavior in #4500 itself as a unique and one-off async instantiation.

I worry though that the UX might be jarring

This is up to the component itself. What is ultimately jarring is seeing a blank page. If for any reason these asynchronous calls fail then the editor is staring at unexplained placeholders. We can encode intentional behaviors in the components based on the presence of these dependencies. For example, in a very simple but still quite effective way we can say "wait up until one second for CodeMirror to load but if it doesn't load after that then just display the <textarea> and add a banner indicating that it failed to load."

the benefits of this kind of progressive enhancement [don't] outweigh the additional complexity involved in implementing it

it's not even additional complexity. the wrapper as written returns Promise.all() but it's probably no more complex and no more simple to just return the array of promises and let the component decide.

withLazyLoaded( {
  'CodeMirror',
  'bestDep'
} )( deps => ( {
  edit( attributes, setAttributes ) {
    Promise.all( deps ).then( () => setAttributes( { allLoaded: true } ) )

    if ( ! allLoaded ) {
      return <Placeholder />;
    }

    // do rest of normal logic
    
  }
} )

Another component may only want enhancements…

withLazyLoaded( {
  'CodeMirror',
  'bestDep'
} )( deps => ( {
  BestDep = null,

  edit( attributes, setAttributes ) {
    deps.bestDep.then(
      dep => BestDep = dep,
      error => setAttributes( { couldNotLoad: error } )
    )

    

    return (
      { attributes.couldNotLoad && <ErrorNotice error={ attributes.couldNotLoad } /> }
      { this.dep && <BestDep /> }
    )
  }
} )

so that code is really sparse and probably really wrong but that's the idea. the promise exposes the lifecycle of the asynchronous load and there is some way to pass those dependencies as props/attributes. even with globals we can still do that when we want by transforming the global into a function parameter. (in the case where the global script does global things and we have nothing else to do that's not needed but then again we don't really need a wrapper for it either).


could it be that for #4500 we don't really need a wrapper component but instead just need a way for Gutenberg to enqueue scripts? that seems like a much simpler problem with less need to understand how the API of a wrapper-component should operate. could add those scripts as properties on the block definition even then Gutenberg could lazy-load them for the blocks.

@noisysocks
Copy link
Member Author

noisysocks commented Jan 24, 2018

Thanks for the detailed notes @dmsnell!

I feel much more hesitant to build this as a generalized system in the wrapper vs. just performing this behavior in #4500 itself as a unique and one-off async instantiation.

Yeah, OK. Perhaps let's punt on introducing withLazyDependencies. You're right that it's hard to say what a good API for such a HOC would be when our only use case right now is this really abnormal CodeEditor component.

could it be that for #4500 we don't really need a wrapper component but instead just need a way for Gutenberg to enqueue scripts?

This is similar to what @aduth brought up in his comment above: we could have Gutenberg asynchronously load the script and style handles that are passed into register_block_type.

I'm just not sure how we'd go about making one of our core block types (the HTML block) use server-side block registration. Would blocks/library/html/index.js still be compiled into the blocks chunk? Or would we have to emit it separately?

could add those scripts as properties on the block definition even then Gutenberg could lazy-load them for the blocks.

My only hesitation with doing this is that it increases the surface area of the blocks API.

@aduth
Copy link
Member

aduth commented Jan 24, 2018

Would blocks/library/html/index.js still be compiled into the blocks chunk? Or would we have to emit it separately?

In my approach, we'd change to have every single block emit its own bundle, then load only the block scripts which are relevant at a given time (editor page load for recent or common blocks, blocks intended to be inserted, etc).

cc @gziolo

@dmsnell
Copy link
Member

dmsnell commented Jan 24, 2018

then load only the block scripts which are relevant at a given time

☝️ because otherwise we wouldn't help ourselves too much - the server would still send the JavaScript even when not opening the editor

@noisysocks
Copy link
Member Author

How do we feel about, for now, lazily loading the WordPress CodeMirror library in a one-off manner? That is, I'll change this PR to remove the reusable HOC. I'd like to get CodeMirror into the HTML block sooner rather than later so that we can collect early feedback on it. Splitting blocks and their dependencies into seperate chunks sounds ideal but it will take some further discussion and time to get the details right.

@noisysocks noisysocks force-pushed the add/syntax-highlighting-to-html-block branch from d9cbfaa to e51f342 Compare January 29, 2018 02:29
@noisysocks
Copy link
Member Author

I implemented lazy loading in a one-off manner over in #4348 (e51f342, specifically). Let's continue discussion over there! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants