From 9baaf0a5cd6e4633332dee0a8bd7b1092ecec24e Mon Sep 17 00:00:00 2001 From: Sasha Aickin Date: Tue, 24 May 2016 16:45:48 -0700 Subject: [PATCH 1/3] Replacing the implementation of escapeTextContentForBrowser with escape-html for performance --- gulpfile.js | 5 +++- package.json | 1 + packages/react/package.json | 1 + scripts/jest/preprocessor.js | 1 + .../__tests__/ReactDOMComponent-test.js | 4 +-- .../dom/shared/escapeTextContentForBrowser.js | 25 ++++++++----------- 6 files changed, 20 insertions(+), 17 deletions(-) diff --git a/gulpfile.js b/gulpfile.js index 452d0362c3c63..bb5928e9b8e32 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -30,7 +30,10 @@ var paths = { }; var moduleMap = Object.assign( - {'object-assign': 'object-assign'}, + { + 'escape-html': 'escape-html', + 'object-assign': 'object-assign', + }, require('fbjs/module-map'), { deepDiffer: 'react-native/lib/deepDiffer', diff --git a/package.json b/package.json index 6397c7e19f506..f75d1aa213cb1 100644 --- a/package.json +++ b/package.json @@ -35,6 +35,7 @@ "coveralls": "^2.11.6", "del": "^2.0.2", "derequire": "^2.0.3", + "escape-html": "1.0.3", "eslint": "1.10.3", "eslint-plugin-react": "4.1.0", "eslint-plugin-react-internal": "file:eslint-rules", diff --git a/packages/react/package.json b/packages/react/package.json index 0c7f04eed8dec..301feab830b69 100644 --- a/packages/react/package.json +++ b/packages/react/package.json @@ -23,6 +23,7 @@ "node": ">=0.10.0" }, "dependencies": { + "escape-html": "1.0.3", "fbjs": "^0.8.1", "loose-envify": "^1.1.0", "object-assign": "^4.1.0" diff --git a/scripts/jest/preprocessor.js b/scripts/jest/preprocessor.js index 0cbc46dd90c1c..c628796a91811 100644 --- a/scripts/jest/preprocessor.js +++ b/scripts/jest/preprocessor.js @@ -32,6 +32,7 @@ var babelOptions = { {}, moduleMap, { + 'escape-html': 'escape-html', 'object-assign': 'object-assign', } ), diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index a0c1db729800f..a2bf1c542921f 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1024,8 +1024,8 @@ describe('ReactDOMComponent', function() { }, '\'"<>&') ) ).toBe( - '
' + - ''"<>&' + + '
' + + ''"<>&' + '
' ); }); diff --git a/src/renderers/dom/shared/escapeTextContentForBrowser.js b/src/renderers/dom/shared/escapeTextContentForBrowser.js index 897daf43ca029..e96749836c28c 100644 --- a/src/renderers/dom/shared/escapeTextContentForBrowser.js +++ b/src/renderers/dom/shared/escapeTextContentForBrowser.js @@ -11,19 +11,7 @@ 'use strict'; -var ESCAPE_LOOKUP = { - '&': '&', - '>': '>', - '<': '<', - '"': '"', - '\'': ''', -}; - -var ESCAPE_REGEX = /[&><"']/g; - -function escaper(match) { - return ESCAPE_LOOKUP[match]; -} +var escapeHtml = require('escape-html'); /** * Escapes text to prevent scripting attacks. @@ -32,7 +20,16 @@ function escaper(match) { * @return {string} An escaped string. */ function escapeTextContentForBrowser(text) { - return ('' + text).replace(ESCAPE_REGEX, escaper); + switch (typeof text) { + case 'boolean': + case 'number': + // this shortcircuit helps perf for types that we know will never have + // special characters, especially given that this function is used often + // for numeric dom ids. + return '' + text; + default: + return escapeHtml(text); + } } module.exports = escapeTextContentForBrowser; From 28b49ead5ad58fa44105c612326bb1ee98a78748 Mon Sep 17 00:00:00 2001 From: Sasha Aickin Date: Wed, 1 Jun 2016 16:34:42 -0700 Subject: [PATCH 2/3] Addressing @spicyj's code review comment here: https://github.com/facebook/react/pull/6862#issuecomment-223102868 . Pulled the code of escape-html in to react and changed the encoding of single quote to '. --- gulpfile.js | 5 +- package.json | 1 - packages/react/package.json | 1 - scripts/jest/preprocessor.js | 1 - .../__tests__/ReactDOMComponent-test.js | 4 +- .../dom/shared/escapeTextContentForBrowser.js | 92 ++++++++++++++++++- 6 files changed, 93 insertions(+), 11 deletions(-) diff --git a/gulpfile.js b/gulpfile.js index bb5928e9b8e32..452d0362c3c63 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -30,10 +30,7 @@ var paths = { }; var moduleMap = Object.assign( - { - 'escape-html': 'escape-html', - 'object-assign': 'object-assign', - }, + {'object-assign': 'object-assign'}, require('fbjs/module-map'), { deepDiffer: 'react-native/lib/deepDiffer', diff --git a/package.json b/package.json index f75d1aa213cb1..6397c7e19f506 100644 --- a/package.json +++ b/package.json @@ -35,7 +35,6 @@ "coveralls": "^2.11.6", "del": "^2.0.2", "derequire": "^2.0.3", - "escape-html": "1.0.3", "eslint": "1.10.3", "eslint-plugin-react": "4.1.0", "eslint-plugin-react-internal": "file:eslint-rules", diff --git a/packages/react/package.json b/packages/react/package.json index 301feab830b69..0c7f04eed8dec 100644 --- a/packages/react/package.json +++ b/packages/react/package.json @@ -23,7 +23,6 @@ "node": ">=0.10.0" }, "dependencies": { - "escape-html": "1.0.3", "fbjs": "^0.8.1", "loose-envify": "^1.1.0", "object-assign": "^4.1.0" diff --git a/scripts/jest/preprocessor.js b/scripts/jest/preprocessor.js index c628796a91811..0cbc46dd90c1c 100644 --- a/scripts/jest/preprocessor.js +++ b/scripts/jest/preprocessor.js @@ -32,7 +32,6 @@ var babelOptions = { {}, moduleMap, { - 'escape-html': 'escape-html', 'object-assign': 'object-assign', } ), diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index a2bf1c542921f..a0c1db729800f 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1024,8 +1024,8 @@ describe('ReactDOMComponent', function() { }, '\'"<>&') ) ).toBe( - '
' + - ''"<>&' + + '
' + + ''"<>&' + '
' ); }); diff --git a/src/renderers/dom/shared/escapeTextContentForBrowser.js b/src/renderers/dom/shared/escapeTextContentForBrowser.js index e96749836c28c..971d7677cc2a8 100644 --- a/src/renderers/dom/shared/escapeTextContentForBrowser.js +++ b/src/renderers/dom/shared/escapeTextContentForBrowser.js @@ -1,17 +1,105 @@ /** - * Copyright 2013-present, Facebook, Inc. + * Copyright 2016-present, Facebook, Inc. * All rights reserved. * * This source code is licensed under the BSD-style license found in the * LICENSE file in the root directory of this source tree. An additional grant * of patent rights can be found in the PATENTS file in the same directory. * + * Based on the escape-html library, which is used under the MIT License below: + * + * Copyright (c) 2012-2013 TJ Holowaychuk + * Copyright (c) 2015 Andreas Lubbe + * Copyright (c) 2015 Tiancheng "Timothy" Gu + * + * Permission is hereby granted, free of charge, to any person obtaining + * a copy of this software and associated documentation files (the + * 'Software'), to deal in the Software without restriction, including + * without limitation the rights to use, copy, modify, merge, publish, + * distribute, sublicense, and/or sell copies of the Software, and to + * permit persons to whom the Software is furnished to do so, subject to + * the following conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED 'AS IS', WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. + * IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY + * CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + * * @providesModule escapeTextContentForBrowser */ 'use strict'; -var escapeHtml = require('escape-html'); +// code copied and modified from escape-html +/** + * Module variables. + * @private + */ + +var matchHtmlRegExp = /["'&<>]/; + +/** + * Escape special characters in the given string of html. + * + * @param {string} string The string to escape for inserting into HTML + * @return {string} + * @public + */ + +function escapeHtml(string) { + var str = '' + string; + var match = matchHtmlRegExp.exec(str); + + if (!match) { + return str; + } + + var escape; + var html = ''; + var index = 0; + var lastIndex = 0; + + for (index = match.index; index < str.length; index++) { + switch (str.charCodeAt(index)) { + case 34: // " + escape = '"'; + break; + case 38: // & + escape = '&'; + break; + case 39: // ' + escape = '''; // modified from escape-html; used to be ''' + break; + case 60: // < + escape = '<'; + break; + case 62: // > + escape = '>'; + break; + default: + continue; + } + + if (lastIndex !== index) { + html += str.substring(lastIndex, index); + } + + lastIndex = index + 1; + html += escape; + } + + return lastIndex !== index + ? html + str.substring(lastIndex, index) + : html; +} +// end code copied and modified from escape-html + /** * Escapes text to prevent scripting attacks. From 97e359ac56d85fc85f2fc0a3b28f48b5e71e9220 Mon Sep 17 00:00:00 2001 From: Sasha Aickin Date: Wed, 1 Jun 2016 16:49:59 -0700 Subject: [PATCH 3/3] Addressing code review comment https://github.com/facebook/react/pull/6862#discussion_r65462074 to make code more inlinable for v8. Thanks, @spicyj. --- .../dom/shared/escapeTextContentForBrowser.js | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/renderers/dom/shared/escapeTextContentForBrowser.js b/src/renderers/dom/shared/escapeTextContentForBrowser.js index 971d7677cc2a8..3735627f823be 100644 --- a/src/renderers/dom/shared/escapeTextContentForBrowser.js +++ b/src/renderers/dom/shared/escapeTextContentForBrowser.js @@ -108,16 +108,13 @@ function escapeHtml(string) { * @return {string} An escaped string. */ function escapeTextContentForBrowser(text) { - switch (typeof text) { - case 'boolean': - case 'number': - // this shortcircuit helps perf for types that we know will never have - // special characters, especially given that this function is used often - // for numeric dom ids. - return '' + text; - default: - return escapeHtml(text); + if (typeof text === 'boolean' || typeof text === 'number') { + // this shortcircuit helps perf for types that we know will never have + // special characters, especially given that this function is used often + // for numeric dom ids. + return '' + text; } + return escapeHtml(text); } module.exports = escapeTextContentForBrowser;