From cb57973392ddb60c3348b31ced6b4d4b19a75af0 Mon Sep 17 00:00:00 2001 From: Christian Kaisermann Date: Mon, 20 Aug 2018 22:39:32 -0300 Subject: [PATCH 1/5] Remove an undefined attribute instead of setting it to "undefined" (string) --- src/shared/dom.js | 8 ++++---- test/runtime/samples/set-undefined-attr/_config.js | 5 +++++ test/runtime/samples/set-undefined-attr/main.html | 14 ++++++++++++++ 3 files changed, 23 insertions(+), 4 deletions(-) create mode 100644 test/runtime/samples/set-undefined-attr/_config.js create mode 100644 test/runtime/samples/set-undefined-attr/main.html diff --git a/src/shared/dom.js b/src/shared/dom.js index d428e92f1c..5e152b9c1b 100644 --- a/src/shared/dom.js +++ b/src/shared/dom.js @@ -82,7 +82,8 @@ export function removeListener(node, event, handler) { } export function setAttribute(node, attribute, value) { - node.setAttribute(attribute, value); + if (value === undefined) removeAttribute(node, attribute); + else node.setAttribute(attribute, value); } export function setAttributes(node, attributes) { @@ -90,8 +91,7 @@ export function setAttributes(node, attributes) { if (key in node) { node[key] = attributes[key]; } else { - if (attributes[key] === undefined) removeAttribute(node, key); - else setAttribute(node, key, attributes[key]); + setAttribute(node, key, attributes[key]); } } } @@ -238,4 +238,4 @@ export function addResizeListener(element, fn) { element.removeChild(object); } }; -} \ No newline at end of file +} diff --git a/test/runtime/samples/set-undefined-attr/_config.js b/test/runtime/samples/set-undefined-attr/_config.js new file mode 100644 index 0000000000..d5aa293bf2 --- /dev/null +++ b/test/runtime/samples/set-undefined-attr/_config.js @@ -0,0 +1,5 @@ +export default { + 'skip-ssr': true, + + html: '
' +}; diff --git a/test/runtime/samples/set-undefined-attr/main.html b/test/runtime/samples/set-undefined-attr/main.html new file mode 100644 index 0000000000..09e63ca45b --- /dev/null +++ b/test/runtime/samples/set-undefined-attr/main.html @@ -0,0 +1,14 @@ +
+ + From c0e7047acad747fa666c562ed4b0dda481b7325e Mon Sep 17 00:00:00 2001 From: Christian Kaisermann Date: Mon, 20 Aug 2018 23:02:24 -0300 Subject: [PATCH 2/5] Also check for null-valued attributes --- src/shared/dom.js | 2 +- .../samples/dont-use-dataset-in-legacy/expected-bundle.js | 7 ++++++- test/js/samples/dont-use-dataset-in-svg/expected-bundle.js | 7 ++++++- test/js/samples/input-files/expected-bundle.js | 7 ++++++- test/js/samples/input-range/expected-bundle.js | 7 ++++++- .../input-without-blowback-guard/expected-bundle.js | 7 ++++++- 6 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/shared/dom.js b/src/shared/dom.js index 5e152b9c1b..51281409a7 100644 --- a/src/shared/dom.js +++ b/src/shared/dom.js @@ -82,7 +82,7 @@ export function removeListener(node, event, handler) { } export function setAttribute(node, attribute, value) { - if (value === undefined) removeAttribute(node, attribute); + if (value == null) removeAttribute(node, attribute); else node.setAttribute(attribute, value); } diff --git a/test/js/samples/dont-use-dataset-in-legacy/expected-bundle.js b/test/js/samples/dont-use-dataset-in-legacy/expected-bundle.js index 30890b83a3..9ca1b19266 100644 --- a/test/js/samples/dont-use-dataset-in-legacy/expected-bundle.js +++ b/test/js/samples/dont-use-dataset-in-legacy/expected-bundle.js @@ -22,7 +22,12 @@ function createText(data) { } function setAttribute(node, attribute, value) { - node.setAttribute(attribute, value); + if (value == null) removeAttribute(node, attribute); + else node.setAttribute(attribute, value); +} + +function removeAttribute(node, attribute) { + node.removeAttribute(attribute); } function blankObject() { diff --git a/test/js/samples/dont-use-dataset-in-svg/expected-bundle.js b/test/js/samples/dont-use-dataset-in-svg/expected-bundle.js index 9c006e2870..4192676418 100644 --- a/test/js/samples/dont-use-dataset-in-svg/expected-bundle.js +++ b/test/js/samples/dont-use-dataset-in-svg/expected-bundle.js @@ -22,7 +22,12 @@ function createSvgElement(name) { } function setAttribute(node, attribute, value) { - node.setAttribute(attribute, value); + if (value == null) removeAttribute(node, attribute); + else node.setAttribute(attribute, value); +} + +function removeAttribute(node, attribute) { + node.removeAttribute(attribute); } function blankObject() { diff --git a/test/js/samples/input-files/expected-bundle.js b/test/js/samples/input-files/expected-bundle.js index 1a6376f193..4e51694625 100644 --- a/test/js/samples/input-files/expected-bundle.js +++ b/test/js/samples/input-files/expected-bundle.js @@ -26,7 +26,12 @@ function removeListener(node, event, handler) { } function setAttribute(node, attribute, value) { - node.setAttribute(attribute, value); + if (value == null) removeAttribute(node, attribute); + else node.setAttribute(attribute, value); +} + +function removeAttribute(node, attribute) { + node.removeAttribute(attribute); } function blankObject() { diff --git a/test/js/samples/input-range/expected-bundle.js b/test/js/samples/input-range/expected-bundle.js index f9b53e91a5..1711ee53bb 100644 --- a/test/js/samples/input-range/expected-bundle.js +++ b/test/js/samples/input-range/expected-bundle.js @@ -26,7 +26,12 @@ function removeListener(node, event, handler) { } function setAttribute(node, attribute, value) { - node.setAttribute(attribute, value); + if (value == null) removeAttribute(node, attribute); + else node.setAttribute(attribute, value); +} + +function removeAttribute(node, attribute) { + node.removeAttribute(attribute); } function toNumber(value) { diff --git a/test/js/samples/input-without-blowback-guard/expected-bundle.js b/test/js/samples/input-without-blowback-guard/expected-bundle.js index c3abad14fe..817aa1e401 100644 --- a/test/js/samples/input-without-blowback-guard/expected-bundle.js +++ b/test/js/samples/input-without-blowback-guard/expected-bundle.js @@ -26,7 +26,12 @@ function removeListener(node, event, handler) { } function setAttribute(node, attribute, value) { - node.setAttribute(attribute, value); + if (value == null) removeAttribute(node, attribute); + else node.setAttribute(attribute, value); +} + +function removeAttribute(node, attribute) { + node.removeAttribute(attribute); } function blankObject() { From a23169262e224bb861d12d14027d0ac8bcbd1b8c Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Sat, 27 Oct 2018 18:00:29 -0400 Subject: [PATCH 3/5] dont render undefined/null attributes in SSR --- src/compile/nodes/Attribute.ts | 18 +++-------- src/compile/render-ssr/handlers/Element.ts | 32 +++++++++++++++---- src/shared/dom.js | 8 ++--- .../samples/set-undefined-attr/_config.js | 4 +-- 4 files changed, 35 insertions(+), 27 deletions(-) diff --git a/src/compile/nodes/Attribute.ts b/src/compile/nodes/Attribute.ts index 83c845176d..6eb3393f40 100644 --- a/src/compile/nodes/Attribute.ts +++ b/src/compile/nodes/Attribute.ts @@ -1,4 +1,4 @@ -import { escape, escapeTemplate, stringify } from '../../utils/stringify'; +import { stringify } from '../../utils/stringify'; import addToSet from '../../utils/addToSet'; import Component from '../Component'; import Node from './shared/Node'; @@ -16,6 +16,7 @@ export default class Attribute extends Node { name: string; isSpread: boolean; isTrue: boolean; + isConcatenated: boolean; isDynamic: boolean; isSynthetic: boolean; shouldCache: boolean; @@ -38,6 +39,7 @@ export default class Attribute extends Node { this.isDynamic = true; // TODO not necessarily this.shouldCache = false; // TODO does this mean anything here? + this.isConcatenated = false; } else { @@ -65,6 +67,8 @@ export default class Attribute extends Node { ? this.chunks[0].node.type !== 'Identifier' || scope.names.has(this.chunks[0].node.name) : true : false; + + this.isConcatenated = this.chunks.length > 1; } } @@ -99,16 +103,4 @@ export default class Attribute extends Node { ? this.chunks[0].data : ''; } - - stringifyForSsr() { - return this.chunks - .map((chunk: Node) => { - if (chunk.type === 'Text') { - return escapeTemplate(escape(chunk.data).replace(/"/g, '"')); - } - - return '${@escape(' + chunk.snippet + ')}'; - }) - .join(''); - } } \ No newline at end of file diff --git a/src/compile/render-ssr/handlers/Element.ts b/src/compile/render-ssr/handlers/Element.ts index 5aec6fb6b5..27954d7418 100644 --- a/src/compile/render-ssr/handlers/Element.ts +++ b/src/compile/render-ssr/handlers/Element.ts @@ -1,5 +1,8 @@ import { quotePropIfNecessary, quoteNameIfNecessary } from '../../../utils/quoteIfNecessary'; import isVoidElementName from '../../../utils/isVoidElementName'; +import Attribute from '../../nodes/Attribute'; +import Node from '../../nodes/shared/Node'; +import { escapeTemplate } from '../../../utils/stringify'; // source: https://gist.github.com/ArjanSchouten/0b8574a6ad7f5065a5e7 const boolean_attributes = new Set([ @@ -71,7 +74,7 @@ export default function(node, renderer, options) { args.push(attribute.expression.snippet); } else { if (attribute.name === 'value' && node.name === 'textarea') { - textareaContents = attribute.stringifyForSsr(); + textareaContents = stringifyAttribute(attribute); } else if (attribute.isTrue) { args.push(`{ ${quoteNameIfNecessary(attribute.name)}: true }`); } else if ( @@ -82,18 +85,18 @@ export default function(node, renderer, options) { // a boolean attribute with one non-Text chunk args.push(`{ ${quoteNameIfNecessary(attribute.name)}: ${attribute.chunks[0].snippet} }`); } else { - args.push(`{ ${quoteNameIfNecessary(attribute.name)}: \`${attribute.stringifyForSsr()}\` }`); + args.push(`{ ${quoteNameIfNecessary(attribute.name)}: \`${stringifyAttribute(attribute)}\` }`); } } }); openingTag += "${@spread([" + args.join(', ') + "])}"; } else { - node.attributes.forEach((attribute: Node) => { + node.attributes.forEach((attribute: Attribute) => { if (attribute.type !== 'Attribute') return; if (attribute.name === 'value' && node.name === 'textarea') { - textareaContents = attribute.stringifyForSsr(); + textareaContents = stringifyAttribute(attribute); } else if (attribute.isTrue) { openingTag += ` ${attribute.name}`; } else if ( @@ -105,9 +108,14 @@ export default function(node, renderer, options) { openingTag += '${' + attribute.chunks[0].snippet + ' ? " ' + attribute.name + '" : "" }'; } else if (attribute.name === 'class' && classExpr) { addClassAttribute = false; - openingTag += ` class="\${[\`${attribute.stringifyForSsr()}\`, ${classExpr}].join(' ').trim() }"`; + openingTag += ` class="\${[\`${stringifyAttribute(attribute)}\`, ${classExpr}].join(' ').trim() }"`; + } else if (attribute.isConcatenated || !attribute.isDynamic) { + openingTag += ` ${attribute.name}="${stringifyAttribute(attribute)}"`; } else { - openingTag += ` ${attribute.name}="${attribute.stringifyForSsr()}"`; + const { name } = attribute; + const { snippet } = attribute.chunks[0]; + + openingTag += '${(v => v == null ? "" : ` ' + name + '=${' + snippet + '}`)(' + snippet + ')}'; } }); } @@ -129,4 +137,16 @@ export default function(node, renderer, options) { if (!isVoidElementName(node.name)) { renderer.append(``); } +} + +function stringifyAttribute(attribute: Attribute) { + return attribute.chunks + .map((chunk: Node) => { + if (chunk.type === 'Text') { + return escapeTemplate(escape(chunk.data).replace(/"/g, '"')); + } + + return '${@escape(' + chunk.snippet + ')}'; + }) + .join(''); } \ No newline at end of file diff --git a/src/shared/dom.js b/src/shared/dom.js index d05bce2e3c..06eb0c410a 100644 --- a/src/shared/dom.js +++ b/src/shared/dom.js @@ -82,7 +82,7 @@ export function removeListener(node, event, handler) { } export function setAttribute(node, attribute, value) { - if (value == null) removeAttribute(node, attribute); + if (value == null) node.removeAttribute(attribute); else node.setAttribute(attribute, value); } @@ -104,14 +104,10 @@ export function setCustomElementData(node, prop, value) { } else if (value) { setAttribute(node, prop, value); } else { - removeAttribute(node, prop); + node.removeAttribute(prop); } } -export function removeAttribute(node, attribute) { - node.removeAttribute(attribute); -} - export function setXlinkAttribute(node, attribute, value) { node.setAttributeNS('http://www.w3.org/1999/xlink', attribute, value); } diff --git a/test/runtime/samples/set-undefined-attr/_config.js b/test/runtime/samples/set-undefined-attr/_config.js index d5aa293bf2..e28bad8257 100644 --- a/test/runtime/samples/set-undefined-attr/_config.js +++ b/test/runtime/samples/set-undefined-attr/_config.js @@ -1,5 +1,5 @@ export default { - 'skip-ssr': true, + html: '
', - html: '
' + ssrHtml: '
' }; From 62fdae5b35a4bf3fa2f8767fc2f60ee9b44d323b Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Sat, 27 Oct 2018 19:11:56 -0400 Subject: [PATCH 4/5] import helper --- src/compile/render-ssr/handlers/Element.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compile/render-ssr/handlers/Element.ts b/src/compile/render-ssr/handlers/Element.ts index 063a4603de..b9aaaa664e 100644 --- a/src/compile/render-ssr/handlers/Element.ts +++ b/src/compile/render-ssr/handlers/Element.ts @@ -2,7 +2,7 @@ import { quotePropIfNecessary, quoteNameIfNecessary } from '../../../utils/quote import isVoidElementName from '../../../utils/isVoidElementName'; import Attribute from '../../nodes/Attribute'; import Node from '../../nodes/shared/Node'; -import { escapeTemplate } from '../../../utils/stringify'; +import { escape, escapeTemplate } from '../../../utils/stringify'; // source: https://gist.github.com/ArjanSchouten/0b8574a6ad7f5065a5e7 const boolean_attributes = new Set([ From 1855ca18fceba0a8d9bd70658b589db941d4c19f Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Sat, 27 Oct 2018 19:42:40 -0400 Subject: [PATCH 5/5] fixes --- src/compile/nodes/Attribute.ts | 4 ---- src/compile/render-ssr/handlers/Element.ts | 8 ++++---- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/compile/nodes/Attribute.ts b/src/compile/nodes/Attribute.ts index 6eb3393f40..0a85d6b850 100644 --- a/src/compile/nodes/Attribute.ts +++ b/src/compile/nodes/Attribute.ts @@ -16,7 +16,6 @@ export default class Attribute extends Node { name: string; isSpread: boolean; isTrue: boolean; - isConcatenated: boolean; isDynamic: boolean; isSynthetic: boolean; shouldCache: boolean; @@ -39,7 +38,6 @@ export default class Attribute extends Node { this.isDynamic = true; // TODO not necessarily this.shouldCache = false; // TODO does this mean anything here? - this.isConcatenated = false; } else { @@ -67,8 +65,6 @@ export default class Attribute extends Node { ? this.chunks[0].node.type !== 'Identifier' || scope.names.has(this.chunks[0].node.name) : true : false; - - this.isConcatenated = this.chunks.length > 1; } } diff --git a/src/compile/render-ssr/handlers/Element.ts b/src/compile/render-ssr/handlers/Element.ts index b9aaaa664e..d7bc823be2 100644 --- a/src/compile/render-ssr/handlers/Element.ts +++ b/src/compile/render-ssr/handlers/Element.ts @@ -109,13 +109,13 @@ export default function(node, renderer, options) { } else if (attribute.name === 'class' && classExpr) { addClassAttribute = false; openingTag += ` class="\${[\`${stringifyAttribute(attribute)}\`, ${classExpr}].join(' ').trim() }"`; - } else if (attribute.isConcatenated || !attribute.isDynamic) { - openingTag += ` ${attribute.name}="${stringifyAttribute(attribute)}"`; - } else { + } else if (attribute.chunks.length === 1 && attribute.chunks[0].type !== 'Text') { const { name } = attribute; const { snippet } = attribute.chunks[0]; - openingTag += '${(v => v == null ? "" : ` ' + name + '=${' + snippet + '}`)(' + snippet + ')}'; + openingTag += '${(v => v == null ? "" : ` ' + name + '="${@escape(' + snippet + ')}"`)(' + snippet + ')}'; + } else { + openingTag += ` ${attribute.name}="${stringifyAttribute(attribute)}"`; } }); }