From 038769c184f3bcf4948faee16e32077d9e8e8e27 Mon Sep 17 00:00:00 2001 From: Andrew Duthie <andrew@andrewduthie.com> Date: Fri, 30 Nov 2018 10:37:05 -0500 Subject: [PATCH 1/4] RichText: Reuse DOM document across calls to createEmpty --- packages/rich-text/CHANGELOG.md | 6 ++++++ packages/rich-text/src/to-dom.js | 23 +++++++++++++++++++++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/packages/rich-text/CHANGELOG.md b/packages/rich-text/CHANGELOG.md index 0e9ef7674dc481..aadcae6d607998 100644 --- a/packages/rich-text/CHANGELOG.md +++ b/packages/rich-text/CHANGELOG.md @@ -1,3 +1,9 @@ +## 3.0.3 (Unreleased) + +### Internal + +- Internal performance optimizations to avoid excessive expensive creation of DOM documents. + ## 3.0.2 (2018-11-21) ## 3.0.1 (2018-11-20) diff --git a/packages/rich-text/src/to-dom.js b/packages/rich-text/src/to-dom.js index b582c77083d9a0..e3c11b6e4b1712 100644 --- a/packages/rich-text/src/to-dom.js +++ b/packages/rich-text/src/to-dom.js @@ -58,9 +58,28 @@ function getNodeByPath( node, path ) { }; } +/** + * Returns a new instance of a DOM tree upon which RichText operations can be + * applied. + * + * Note: The current implementation will return a shared reference, reset on + * each call to `createEmpty`. Therefore, you should not hold a reference to + * the value to operate upon asynchronously, as it may have unexpected results. + * + * @return {WPRichTextTree} RichText tree. + */ function createEmpty() { - const { body } = document.implementation.createHTMLDocument( '' ); - return body; + // Because `createHTMLDocument` is an expensive operation, and with this + // function being internal to `rich-text` (full control in avoiding a risk + // of asynchronous operations on the shared reference), a single document + // is reused and reset for each call to the function. + if ( createEmpty.body ) { + createEmpty.body.innerHTML = ''; + } else { + createEmpty.body = document.implementation.createHTMLDocument( '' ).body; + } + + return createEmpty.body; } function append( element, child ) { From f0c9aa5647c455189ef35a48b26d73d8f43e3823 Mon Sep 17 00:00:00 2001 From: iseulde <wp@iseulde.com> Date: Fri, 30 Nov 2018 16:26:27 +0100 Subject: [PATCH 2/4] RichText: Reuse document in createElement --- packages/rich-text/src/create-element.js | 14 +++++++++++--- packages/rich-text/src/test/to-dom.js | 7 ++++++- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/packages/rich-text/src/create-element.js b/packages/rich-text/src/create-element.js index 0e2f6f3572fdf8..6a174a4c15562d 100644 --- a/packages/rich-text/src/create-element.js +++ b/packages/rich-text/src/create-element.js @@ -1,13 +1,21 @@ /** * Parse the given HTML into a body element. * + * Note: The current implementation will return a shared reference, reset on + * each call to `createElement`. Therefore, you should not hold a reference to + * the value to operate upon asynchronously, as it may have unexpected results. + * * @param {HTMLDocument} document The HTML document to use to parse. * @param {string} html The HTML to parse. * * @return {HTMLBodyElement} Body element with parsed HTML. */ export function createElement( { implementation }, html ) { - const { body } = implementation.createHTMLDocument( '' ); - body.innerHTML = html; - return body; + if ( ! createElement.body ) { + createElement.body = implementation.createHTMLDocument( '' ).body; + } + + createElement.body.innerHTML = html; + + return createElement.body; } diff --git a/packages/rich-text/src/test/to-dom.js b/packages/rich-text/src/test/to-dom.js index 92cc47aa28846e..ed16daf1b38c30 100644 --- a/packages/rich-text/src/test/to-dom.js +++ b/packages/rich-text/src/test/to-dom.js @@ -9,12 +9,17 @@ import { JSDOM } from 'jsdom'; */ import { toDom, applyValue } from '../to-dom'; -import { createElement } from '../create-element'; import { spec } from './helpers'; const { window } = new JSDOM(); const { document } = window; +function createElement( { implementation }, html ) { + const { body } = implementation.createHTMLDocument( '' ); + body.innerHTML = html; + return body; +} + describe( 'recordToDom', () => { beforeAll( () => { // Initialize the rich-text store. From bdce8465ff9d80cdd09fa9ed7628c36e0d9605ec Mon Sep 17 00:00:00 2001 From: Andrew Duthie <andrew@andrewduthie.com> Date: Fri, 30 Nov 2018 10:48:05 -0500 Subject: [PATCH 3/4] Rich Text: Update tests to clone node to retain reference --- packages/rich-text/src/test/to-dom.js | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/rich-text/src/test/to-dom.js b/packages/rich-text/src/test/to-dom.js index ed16daf1b38c30..16fed3f66d0a26 100644 --- a/packages/rich-text/src/test/to-dom.js +++ b/packages/rich-text/src/test/to-dom.js @@ -9,17 +9,12 @@ import { JSDOM } from 'jsdom'; */ import { toDom, applyValue } from '../to-dom'; +import { createElement } from '../create-element'; import { spec } from './helpers'; const { window } = new JSDOM(); const { document } = window; -function createElement( { implementation }, html ) { - const { body } = implementation.createHTMLDocument( '' ); - body.innerHTML = html; - return body; -} - describe( 'recordToDom', () => { beforeAll( () => { // Initialize the rich-text store. @@ -71,8 +66,8 @@ describe( 'applyValue', () => { cases.forEach( ( { current, future, description, movedCount } ) => { it( description, () => { - const body = createElement( document, current ); - const futureBody = createElement( document, future ); + const body = createElement( document, current ).cloneNode( true ); + const futureBody = createElement( document, future ).cloneNode( true ); const childNodes = Array.from( futureBody.childNodes ); applyValue( futureBody, body ); const count = childNodes.reduce( ( acc, { parentNode } ) => { From f9e9e89f37950c9d07609ccd2d05222bac69e755 Mon Sep 17 00:00:00 2001 From: Andrew Duthie <andrew@andrewduthie.com> Date: Fri, 30 Nov 2018 10:51:00 -0500 Subject: [PATCH 4/4] Rich Text: Refactor createEmpty to use createElement --- packages/rich-text/src/create-element.js | 4 ++++ packages/rich-text/src/to-dom.js | 15 ++------------- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/packages/rich-text/src/create-element.js b/packages/rich-text/src/create-element.js index 6a174a4c15562d..eebbe450075e80 100644 --- a/packages/rich-text/src/create-element.js +++ b/packages/rich-text/src/create-element.js @@ -11,6 +11,10 @@ * @return {HTMLBodyElement} Body element with parsed HTML. */ export function createElement( { implementation }, html ) { + // Because `createHTMLDocument` is an expensive operation, and with this + // function being internal to `rich-text` (full control in avoiding a risk + // of asynchronous operations on the shared reference), a single document + // is reused and reset for each call to the function. if ( ! createElement.body ) { createElement.body = implementation.createHTMLDocument( '' ).body; } diff --git a/packages/rich-text/src/to-dom.js b/packages/rich-text/src/to-dom.js index e3c11b6e4b1712..1c34d8680dd2b3 100644 --- a/packages/rich-text/src/to-dom.js +++ b/packages/rich-text/src/to-dom.js @@ -3,6 +3,7 @@ */ import { toTree } from './to-tree'; +import { createElement } from './create-element'; /** * Browser dependencies @@ -68,19 +69,7 @@ function getNodeByPath( node, path ) { * * @return {WPRichTextTree} RichText tree. */ -function createEmpty() { - // Because `createHTMLDocument` is an expensive operation, and with this - // function being internal to `rich-text` (full control in avoiding a risk - // of asynchronous operations on the shared reference), a single document - // is reused and reset for each call to the function. - if ( createEmpty.body ) { - createEmpty.body.innerHTML = ''; - } else { - createEmpty.body = document.implementation.createHTMLDocument( '' ).body; - } - - return createEmpty.body; -} +const createEmpty = () => createElement( document, '' ); function append( element, child ) { if ( typeof child === 'string' ) {