From 8c3174137b64d03c778ce8b7a9e22a1cf3d60c82 Mon Sep 17 00:00:00 2001 From: Ryan Cebulko Date: Tue, 23 Feb 2021 12:51:53 -0500 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Fixit:=20standalone=20asse?= =?UTF-8?q?rt=20helpers=20w/=20format=20messages=20(#32788)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * support formattrings in asserts * Add tests for pure-assert * Import as pure asserts * Update date.js to use format args * rename pureAssertion to assertion * Rename `pure-assert.js` to `assert.js` * Fix test file * Fix copyright year Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> * fix assertion * Lint fixes Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> --- extensions/amp-accordion/1.0/base-element.js | 4 +- extensions/amp-selector/1.0/base-element.js | 12 +- src/assert.js | 172 +++++++++++++++++++ src/log.js | 2 +- src/pure-assert.js | 94 ---------- src/utils/date.js | 19 +- test/unit/test-assert.js | 64 +++++++ 7 files changed, 255 insertions(+), 112 deletions(-) create mode 100644 src/assert.js delete mode 100644 src/pure-assert.js create mode 100644 test/unit/test-assert.js diff --git a/extensions/amp-accordion/1.0/base-element.js b/extensions/amp-accordion/1.0/base-element.js index 80e3e930360b..1b1ef6be89d5 100644 --- a/extensions/amp-accordion/1.0/base-element.js +++ b/extensions/amp-accordion/1.0/base-element.js @@ -23,9 +23,9 @@ import { } from './component'; import {PreactBaseElement} from '../../../src/preact/base-element'; import {childElementsByTag, toggleAttribute} from '../../../src/dom'; +import {pureDevAssert as devAssert} from '../../../src/assert'; import {dict, memo} from '../../../src/utils/object'; import {forwardRef} from '../../../src/preact/compat'; -import {pureDevAssert} from '../../../src/pure-assert'; import {toArray} from '../../../src/types'; import { useImperativeHandle, @@ -175,7 +175,7 @@ function HeaderShim( sectionElement[SECTION_POST_RENDER](); } return () => { - headerElement.removeEventListener('click', pureDevAssert(onClick)); + headerElement.removeEventListener('click', devAssert(onClick)); }; }, [ sectionElement, diff --git a/extensions/amp-selector/1.0/base-element.js b/extensions/amp-selector/1.0/base-element.js index 255e83c39db2..fe91b3b4f9a6 100644 --- a/extensions/amp-selector/1.0/base-element.js +++ b/extensions/amp-selector/1.0/base-element.js @@ -23,8 +23,8 @@ import { toggleAttribute, tryFocus, } from '../../../src/dom'; +import {pureDevAssert as devAssert} from '../../../src/assert'; import {dict} from '../../../src/utils/object'; -import {pureDevAssert} from '../../../src/pure-assert'; import {toArray} from '../../../src/types'; import {useCallback, useLayoutEffect, useRef} from '../../../src/preact'; @@ -96,10 +96,7 @@ function getOptions(element, mu) { .filter( (el) => !closestAncestorElementBySelector( - pureDevAssert( - el.parentElement?.nodeType == 1 && el.parentElement, - 'Expected an element' - ), + devAssert(el.parentElement?.nodeType == 1, 'Expected an element'), '[option]' ) ) @@ -155,8 +152,7 @@ export function OptionShim({ return; } shimDomElement.addEventListener(type, handler); - return () => - shimDomElement.removeEventListener(type, pureDevAssert(handler)); + return () => shimDomElement.removeEventListener(type, devAssert(handler)); }, [shimDomElement] ); @@ -241,7 +237,7 @@ function SelectorShim({ } shimDomElement.addEventListener('keydown', onKeyDown); return () => - shimDomElement.removeEventListener('keydown', pureDevAssert(onKeyDown)); + shimDomElement.removeEventListener('keydown', devAssert(onKeyDown)); }, [shimDomElement, onKeyDown]); useLayoutEffect(() => { diff --git a/src/assert.js b/src/assert.js new file mode 100644 index 000000000000..9e448c7e0b88 --- /dev/null +++ b/src/assert.js @@ -0,0 +1,172 @@ +/** + * Copyright 2021 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** @fileoverview Dependency-free assertion helpers for use in Preact. */ + +/** + * Triple zero width space. + * + * This is added to user error messages, so that we can later identify + * them, when the only thing that we have is the message. This is the + * case in many browsers when the global exception handler is invoked. + * + * @const {string} + */ +export const USER_ERROR_SENTINEL = '\u200B\u200B\u200B'; + +/** + * User error class for use in Preact. Use of sentinel string instead of a + * boolean to check user errors because errors could be rethrown by some native + * code as a new error, and only a message would survive. Mirrors errors + * produced by `user().error()` in src/log.js. + * @final + * @public + */ +export class UserError extends Error { + /** + * Builds the error, adding the user sentinel if not present. + * @param {string} message + */ + constructor(message) { + if (!message) { + message = USER_ERROR_SENTINEL; + } else if (message.indexOf(USER_ERROR_SENTINEL) == -1) { + message += USER_ERROR_SENTINEL; + } + + super(message); + } +} + +/** + * Throws a provided error if the second argument isn't trueish. + * @param {Object} errorCls + * @param {T} shouldBeTruthy + * @param {string} opt_message + * @param {...*} var_args Arguments substituted into %s in the message + * @return {T} + * @throws {Error} when shouldBeTruthy is not truthy. + */ +function assertion(errorCls, shouldBeTruthy, opt_message, var_args) { + if (shouldBeTruthy) { + return shouldBeTruthy; + } + + // Substitute provided values into format string in message + const message = Array.prototype.slice + // Skip the first 3 arguments to isolate format params + .call(arguments, 3) + .reduce( + (msg, subValue) => msg.replace('%s', subValue), + opt_message || 'Assertion failed' + ); + + throw new errorCls(message); +} + +/** + * Throws a user error if the first argument isn't trueish. Mirrors userAssert + * in src/log.js. + * @param {T} shouldBeTruthy + * @param {string} opt_message + * @param {*=} opt_1 Optional argument (var arg as individual params for better + * @param {*=} opt_2 Optional argument inlining) + * @param {*=} opt_3 Optional argument + * @param {*=} opt_4 Optional argument + * @param {*=} opt_5 Optional argument + * @param {*=} opt_6 Optional argument + * @param {*=} opt_7 Optional argument + * @param {*=} opt_8 Optional argument + * @param {*=} opt_9 Optional argument + * @return {T} + * @throws {UserError} when shouldBeTruthy is not truthy. + * @closurePrimitive {asserts.truthy} + */ +export function pureUserAssert( + shouldBeTruthy, + opt_message, + opt_1, + opt_2, + opt_3, + opt_4, + opt_5, + opt_6, + opt_7, + opt_8, + opt_9 +) { + return assertion( + UserError, + shouldBeTruthy, + opt_message, + opt_1, + opt_2, + opt_3, + opt_4, + opt_5, + opt_6, + opt_7, + opt_8, + opt_9 + ); +} + +/** + * Throws an error if the first argument isn't trueish. Mirrors devAssert in + * src/log.js. + * @param {T} shouldBeTruthy + * @param {string} opt_message + * @param {*=} opt_1 Optional argument (var arg as individual params for better + * @param {*=} opt_2 Optional argument inlining) + * @param {*=} opt_3 Optional argument + * @param {*=} opt_4 Optional argument + * @param {*=} opt_5 Optional argument + * @param {*=} opt_6 Optional argument + * @param {*=} opt_7 Optional argument + * @param {*=} opt_8 Optional argument + * @param {*=} opt_9 Optional argument + * @return {T} + * @throws {Error} when shouldBeTruthy is not truthy. + * @closurePrimitive {asserts.truthy} + */ +export function pureDevAssert( + shouldBeTruthy, + opt_message, + opt_1, + opt_2, + opt_3, + opt_4, + opt_5, + opt_6, + opt_7, + opt_8, + opt_9 +) { + return assertion( + Error, + shouldBeTruthy, + opt_message, + opt_1, + opt_2, + opt_3, + opt_4, + opt_5, + opt_6, + opt_7, + opt_8, + opt_9 + ); +} diff --git a/src/log.js b/src/log.js index 06901f67ae89..4ba8716eafb5 100644 --- a/src/log.js +++ b/src/log.js @@ -14,7 +14,7 @@ * limitations under the License. */ -import {USER_ERROR_SENTINEL} from './pure-assert'; +import {USER_ERROR_SENTINEL} from './assert'; import {getMode} from './mode'; import {internalRuntimeVersion} from './internal-version'; import {isArray, isEnumValue} from './types'; diff --git a/src/pure-assert.js b/src/pure-assert.js deleted file mode 100644 index ffe570443cb3..000000000000 --- a/src/pure-assert.js +++ /dev/null @@ -1,94 +0,0 @@ -/** - * Copyright 2021 The AMP HTML Authors. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS-IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -/** @fileoverview Dependency-free assertion helpers for use in Preact. */ - -/** - * Triple zero width space. - * - * This is added to user error messages, so that we can later identify - * them, when the only thing that we have is the message. This is the - * case in many browsers when the global exception handler is invoked. - * - * @const {string} - */ -export const USER_ERROR_SENTINEL = '\u200B\u200B\u200B'; - -/** - * User error class for use in Preact. Use of sentinel string instead of a - * boolean to check user errors because errors could be rethrown by some native - * code as a new error, and only a message would survive. Mirrors errors - * produced by `user().error()` in src/log.js. - * @final - * @public - */ -export class UserError extends Error { - /** - * Builds the error, adding the user sentinel if not present. - * @param {string} message - */ - constructor(message) { - if (!message) { - message = USER_ERROR_SENTINEL; - } else if (message.indexOf(USER_ERROR_SENTINEL) == -1) { - message += USER_ERROR_SENTINEL; - } - - super(message); - } -} - -/** - * Throws a provided error if the second argument isn't trueish. - * @param {Object} errorCls - * @param {T} shouldBeTruthy - * @param {string} message - * @return {T} - * @throws {Error} when attribute values are missing or invalid. - */ -function pureAssertion(errorCls, shouldBeTruthy, message) { - // TODO: Support format strings. - if (!shouldBeTruthy) { - throw new errorCls(message); - } - return shouldBeTruthy; -} - -/** - * Throws a user error if the first argument isn't trueish. Mirrors userAssert - * in src/log.js. - * @param {T} shouldBeTruthy - * @param {string} message - * @return {T} - * @throws {UserError} when attribute values are missing or invalid. - * @closurePrimitive {asserts.truthy} - */ -export function pureUserAssert(shouldBeTruthy, message) { - return pureAssertion(UserError, shouldBeTruthy, message); -} - -/** - * Throws an error if the first argument isn't trueish. Mirrors devAssert in - * src/log.js. - * @param {T} shouldBeTruthy - * @param {string} message - * @return {T} - * @throws {Error} when attribute values are missing or invalid. - * @closurePrimitive {asserts.truthy} - */ -export function pureDevAssert(shouldBeTruthy, message) { - return pureAssertion(Error, shouldBeTruthy, message); -} diff --git a/src/utils/date.js b/src/utils/date.js index 3540aba0e24f..0b864d23cae8 100644 --- a/src/utils/date.js +++ b/src/utils/date.js @@ -14,7 +14,10 @@ * limitations under the License. */ -import {pureDevAssert, pureUserAssert} from '../pure-assert'; +import { + pureDevAssert as devAssert, + pureUserAssert as userAssert, +} from '../assert'; /** * Parses the date using the `Date.parse()` rules. Additionally supports the @@ -57,9 +60,9 @@ export function getDate(value) { /** Map from attribute names to their parsers. */ const dateAttrParsers = { 'datetime': (datetime) => - pureUserAssert(parseDate(datetime), `Invalid date: ${datetime}`), + userAssert(parseDate(datetime), 'Invalid date: %s', datetime), 'end-date': (datetime) => - pureUserAssert(parseDate(datetime), `Invalid date: ${datetime}`), + userAssert(parseDate(datetime), 'Invalid date: %s', datetime), 'timeleft-ms': (timeleftMs) => Date.now() + Number(timeleftMs), 'timestamp-ms': (ms) => Number(ms), 'timestamp-seconds': (timestampSeconds) => 1000 * Number(timestampSeconds), @@ -71,9 +74,10 @@ const dateAttrParsers = { * @return {?number} */ export function parseDateAttrs(element, dateAttrs) { - const epoch = pureUserAssert( + const epoch = userAssert( parseEpoch(element, dateAttrs), - `One of [${dateAttrs.join(', ')}] is required` + 'One of attributes [%s] is required', + dateAttrs.join(', ') ); const offsetSeconds = @@ -93,9 +97,10 @@ function parseEpoch(element, dateAttrs) { // invalid attr is provided, even if that attribute isn't present on the // element. const parsers = dateAttrs.map((attrName) => - pureDevAssert( + devAssert( dateAttrParsers[attrName], - `Invalid date attribute "${attrName}"` + 'Invalid date attribute "%s"', + attrName ) ); diff --git a/test/unit/test-assert.js b/test/unit/test-assert.js new file mode 100644 index 000000000000..205551562486 --- /dev/null +++ b/test/unit/test-assert.js @@ -0,0 +1,64 @@ +/** + * Copyright 2021 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { + USER_ERROR_SENTINEL, + pureDevAssert as devAssert, + pureUserAssert as userAssert, +} from '../../src/assert'; + +describes.sandboxed('assertions', {}, () => { + describe('devAssert', () => { + it('should not fail for truthy values', () => { + devAssert(true, 'True!'); + devAssert(1, '1'); + devAssert('abc', 'abc'); + }); + + it('should fail for falsey values dev', () => { + expect(() => devAssert(false, 'xyz')).to.throw('xyz'); + expect(() => userAssert(false, '123')).to.throw( + `123${USER_ERROR_SENTINEL}` + ); + }); + }); + + describe('userAssert', () => { + it('should not fail for truthy values', () => { + userAssert(true, 'True!'); + userAssert(1, '1'); + userAssert('abc', 'abc'); + }); + + it('should fail for falsey values dev', () => { + expect(() => userAssert(false, 'xyz')).to.throw( + `xyz${USER_ERROR_SENTINEL}` + ); + }); + }); + + it('should substitute', () => { + expect(() => devAssert(false, 'should fail %s', 'XYZ')).to.throw( + 'should fail XYZ' + ); + expect(() => devAssert(false, 'should fail %s %s', 'XYZ', 'YYY')).to.throw( + 'should fail XYZ YYY' + ); + expect(() => userAssert(false, '%s a %s b %s', 1, 2, 3)).to.throw( + `1 a 2 b 3${USER_ERROR_SENTINEL}` + ); + }); +});