From 74a750d239f737c2939f30962a05089fad2c60ad Mon Sep 17 00:00:00 2001 From: Tim Branyen Date: Fri, 4 Jun 2021 13:55:24 -0700 Subject: [PATCH] Add unit tests and improve config --- packages/diffhtml/lib/node/patch.js | 68 ++++++++-------------------- packages/diffhtml/lib/tree/create.js | 5 +- packages/diffhtml/lib/util/config.js | 29 ++++++------ packages/diffhtml/test/tree.js | 14 ++++++ packages/diffhtml/test/util.js | 66 +++++++++++++++++++++++++++ 5 files changed, 116 insertions(+), 66 deletions(-) diff --git a/packages/diffhtml/lib/node/patch.js b/packages/diffhtml/lib/node/patch.js index f3c41994..f1d0177a 100644 --- a/packages/diffhtml/lib/node/patch.js +++ b/packages/diffhtml/lib/node/patch.js @@ -17,8 +17,8 @@ const blocklist = new Set(); const allowlist = new Set(); /** - * Directly set this attributes, in addition to setting as properties. - * @type {any} + * Directly set these attributes in addition to setting as properties. + * @type {string[]} */ const DIRECT = ['class', 'checked', 'disabled', 'selected']; @@ -69,9 +69,7 @@ const setAttribute = (vTree, domNode, name, value) => { const noValue = value === null || value === undefined; // If we cannot set the value as a property, try as an attribute. - if (lowerName) { - htmlElement.setAttribute(lowerName, noValue ? EMPTY.STR : value); - } + htmlElement.setAttribute(lowerName, noValue ? EMPTY.STR : value); } // Support patching an object representation of the style object. else if (isObject && lowerName === 'style') { @@ -97,15 +95,14 @@ const removeAttribute = (domNode, name) => { // Runtime checking if the property can be set. const blocklistName = /** @type {HTMLElement} */ (domNode).nodeName + '-' + name; + const anyNode = /** @type {any} */ (domNode); - if (allowlist.has(blocklistName)) { - const anyNode = /** @type {any} */ (domNode); + if (isEvent) { + anyNode[name] = undefined; + } - if (isEvent) { - anyNode[name] = undefined; - } else { - delete anyNode[name]; - } + if (allowlist.has(blocklistName)) { + delete anyNode[name]; if (DIRECT.includes(name)) { /** @type {any} */ (domNode)[name] = false; @@ -113,12 +110,7 @@ const removeAttribute = (domNode, name) => { } else if (!blocklist.has(blocklistName)) { try { - const anyNode = /** @type {any} */ (domNode); - if (isEvent) { - anyNode[name] = undefined; - } else { - delete anyNode[name]; - } + delete anyNode[name]; if (DIRECT.includes(name)) { /** @type {any} */ (domNode)[name] = false; @@ -184,10 +176,6 @@ export default function patchNode(patches, state = EMPTY.OBJ) { createNode(vTree, ownerDocument, isSVG) ); - if (!domNode) { - break; - } - const oldValue = domNode.getAttribute(name); const attributeChangedPromises = runTransitions( 'attributeChanged', vTree, name, oldValue, value @@ -219,10 +207,6 @@ export default function patchNode(patches, state = EMPTY.OBJ) { createNode(vTree, ownerDocument, isSVG) ); - if (!domNode) { - break; - } - const oldValue = domNode.getAttribute(name); const attributeChangedPromises = runTransitions( 'attributeChanged', vTree, name, oldValue, null @@ -255,10 +239,6 @@ export default function patchNode(patches, state = EMPTY.OBJ) { createNode(vTree, ownerDocument, isSVG) ); - if (!domNode) { - break; - } - protectVTree(vTree); const textChangedPromises = runTransitions( @@ -285,18 +265,16 @@ export default function patchNode(patches, state = EMPTY.OBJ) { i += 4; + if (!NodeCache.has(vTree)) { + continue; + } + // First attempt to locate a pre-existing DOM Node. If one hasn't been // created there could be a few reasons. let domNode = /** @type {HTMLElement} */ ( NodeCache.get(vTree) ); - // Patching without an existing DOM Node is a mistake, so we should not - // attempt to do anything in this case. - if (!domNode) { - break; - } - const isSVG = svgElements.has(newTree); protectVTree(newTree); @@ -352,18 +330,14 @@ export default function patchNode(patches, state = EMPTY.OBJ) { // the constraints. If there are no transitions, do not use the more // expensive, but more expressive path. if (!hasAttached && !hasDetached && !hasReplaced) { - if (oldDomNode.parentNode) { - oldDomNode.parentNode.replaceChild(newDomNode, oldDomNode); - unprotectVTree(oldTree); - } + oldDomNode.parentNode.replaceChild(newDomNode, oldDomNode); + unprotectVTree(oldTree); break; } // Always insert before to allow the element to transition. - if (oldDomNode.parentNode) { - oldDomNode.parentNode.insertBefore(newDomNode, oldDomNode); - } + oldDomNode.parentNode.insertBefore(newDomNode, oldDomNode); const allPromises = [ ...(hasAttached && runTransitions('attached', newTree) || EMPTY.ARR), @@ -383,9 +357,7 @@ export default function patchNode(patches, state = EMPTY.OBJ) { promises.push(...allPromises); } else { - if (oldDomNode.parentNode) { - oldDomNode.parentNode.removeChild(oldDomNode); - } + oldDomNode.parentNode.removeChild(oldDomNode); unprotectVTree(oldTree); } @@ -416,9 +388,7 @@ export default function patchNode(patches, state = EMPTY.OBJ) { promises.push(...detachedPromises); } else { - if (domNode.parentNode) { - domNode.parentNode.removeChild(domNode); - } + domNode.parentNode.removeChild(domNode); unprotectVTree(vTree); } diff --git a/packages/diffhtml/lib/tree/create.js b/packages/diffhtml/lib/tree/create.js index 47a417f7..6f5b7704 100644 --- a/packages/diffhtml/lib/tree/create.js +++ b/packages/diffhtml/lib/tree/create.js @@ -44,8 +44,9 @@ export default function createTree(input, attributes, childNodes, ...rest) { // When using an Array copy the Nodes in and ensure a valid top-level tree. for (let i = 0; i < length; i++) { - if (!input) continue; - childNodes.push(input[i]); + const hasInput = input && !input[i]; + if (hasInput) continue; + input && childNodes.push(input[i]); } entry = createTree(fragmentName, null, childNodes); diff --git a/packages/diffhtml/lib/util/config.js b/packages/diffhtml/lib/util/config.js index f63271e5..97bf1a98 100644 --- a/packages/diffhtml/lib/util/config.js +++ b/packages/diffhtml/lib/util/config.js @@ -4,11 +4,6 @@ import globalThis from './global'; const { parseInt } = Number; const { parse } = JSON; -const { location, URLSearchParams } = globalThis; -const hasSearchParams = typeof URLSearchParams !== 'undefined'; -const hasLocation = typeof location !== 'undefined'; -const useSearchParams = hasSearchParams && hasLocation; -const useEnv = process.env; /** @type {Config} */ export const globalConfig = { @@ -40,10 +35,6 @@ function formatValue(value, type) { return parseInt(valueAsString, 10); } - case 'array': { - return valueAsString.split(','); - } - case 'object': { return parse(valueAsString); } @@ -64,6 +55,12 @@ function formatValue(value, type) { * @return {unknown} */ export default function getConfig(name, defaultValue, type = typeof defaultValue, overrides) { + const { location, URLSearchParams } = globalThis; + const hasSearchParams = typeof URLSearchParams !== 'undefined'; + const hasLocation = typeof location !== 'undefined'; + const useSearchParams = hasSearchParams && hasLocation; + const useEnv = process.env; + // Allow bypassing any lookups if overrides are passed and match the config // being looked up. if (overrides && name in overrides) { @@ -72,22 +69,24 @@ export default function getConfig(name, defaultValue, type = typeof defaultValue // The keyname for lookups via search params or env variable is DIFF_key and // is case-insensitive. This is why we lowercaes the entire lookup. - const keyName = `DIFF_${name.replace(/[^a-zA-Z0-9]/, '')}`.toLowerCase(); + const keyName = `DIFF_${name.replace(/[^a-zA-Z0-9]/, '')}`; // Try URL search params first. if (useSearchParams) { const searchParams = new URLSearchParams(location.search); + const lowerKey = keyName.toLowerCase(); // Use has here, because boolean values can be set with only a key. - if (searchParams.has(keyName)) { - return formatValue(decodeURIComponent(String(searchParams.get(keyName))), type); + if (searchParams.has(lowerKey)) { + return formatValue(decodeURIComponent(String(searchParams.get(lowerKey))), type); } } // Try environment variables. - if (useEnv && keyName in process.env) { - return formatValue(process.env[keyName], type); + const upperKey = keyName.toUpperCase(); + if (useEnv && upperKey in process.env) { + return formatValue(process.env[upperKey.toUpperCase()], type); } return defaultValue; -} \ No newline at end of file +} diff --git a/packages/diffhtml/test/tree.js b/packages/diffhtml/test/tree.js index 95f90876..58e89faa 100644 --- a/packages/diffhtml/test/tree.js +++ b/packages/diffhtml/test/tree.js @@ -90,6 +90,20 @@ describe('Tree', function() { }); }); + it('will ignore falsy values when creating fragment from an array', () => { + const vTree = createTree([createTree('div'), null, createTree('p')]); + + deepStrictEqual(vTree, { + rawNodeName: '#document-fragment', + nodeName: '#document-fragment', + nodeValue: '', + nodeType: 11, + key: '', + attributes: {}, + childNodes: [createTree('div'), createTree('p')], + }); + }); + it('will ignore falsy values when creating text from an array', () => { const vTree = createTree('#text', ['some text', null, ' ', 'chunks']); diff --git a/packages/diffhtml/test/util.js b/packages/diffhtml/test/util.js index ca4bcb59..d11cc3a6 100644 --- a/packages/diffhtml/test/util.js +++ b/packages/diffhtml/test/util.js @@ -9,10 +9,12 @@ import _process from '../lib/util/process'; import { protectVTree, unprotectVTree, gc } from '../lib/util/memory'; import makeMeasure from '../lib/util/make-measure'; import Pool from '../lib/util/pool'; +import getConfig from '../lib/util/config'; import validateMemory from './util/validate-memory'; import createSupplemental from './util/create-supplemental'; const { floor } = Math; +const { stringify } = JSON; describe('Util', function() { let performance; @@ -34,7 +36,71 @@ describe('Util', function() { }); describe('Config', () => { + it('will load boolean type from process.env', () => { + process.env.DIFF_TESTBOOL = 'true'; + const actual = getConfig('test_bool', false); + strictEqual(actual, true); + }); + + it('will load boolean type from search params', () => { + location.search = '?diff_testbool=true'; + + const actual = getConfig('test_bool', false); + strictEqual(actual, true); + }); + + it('will load string type from process.env', () => { + process.env.DIFF_TESTSTR = 'some-str'; + + const actual = getConfig('test_str', ''); + strictEqual(actual, 'some-str'); + }); + + it('will load string type from search params', () => { + location.search = '?diff_teststr=test-str'; + + const actual = getConfig('test_str', ''); + strictEqual(actual, 'some-str'); + }); + + it('will load number type from process.env', () => { + process.env.DIFF_TESTNUM = '144'; + + const actual = getConfig('test_num', -1); + strictEqual(actual, 144); + }); + + it('will load number type from search params', () => { + location.search = '?diff_testnum=144'; + + const actual = getConfig('test_num', -1); + strictEqual(actual, 144); + }); + + it('will load object type from process.env', () => { + process.env.DIFF_TESTOBJ = stringify({ 1: true, 2: false }); + + const actual = getConfig('test_obj', {}); + deepStrictEqual(actual, { 1: true, 2: false }); + }); + + it('will load object type from search params', () => { + location.search = `?diff_testobj=${stringify({ 1: true, 2: false })}`; + + const actual = getConfig('test_obj', {}); + deepStrictEqual(actual, { 1: true, 2: false }); + }); + + it('will not error if not in a browser env', () => { + const oldLocation = global.location; + + delete global.location; + + strictEqual(getConfig('t', 1), 1); + + global.location = oldLocation; + }); }); describe('DecodeEntities', () => {