From a1d86daa71409d8775fea1e9da13bcfe6aca772c Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Mon, 12 Feb 2018 22:46:15 -0800 Subject: [PATCH] refactor(ivy): assertion (#22189) Encourage the use of message to explain the assertion PR Close #22189 --- packages/core/src/render3/assert.ts | 72 ++++++++----------- packages/core/src/render3/component.ts | 6 +- packages/core/src/render3/instructions.ts | 55 ++++++++------ packages/core/src/render3/node_assert.ts | 23 +++--- .../core/src/render3/node_selector_matcher.ts | 2 +- packages/core/src/render3/query.ts | 2 +- 6 files changed, 77 insertions(+), 83 deletions(-) diff --git a/packages/core/src/render3/assert.ts b/packages/core/src/render3/assert.ts index 14e9556276..0b1bbc7cea 100644 --- a/packages/core/src/render3/assert.ts +++ b/packages/core/src/render3/assert.ts @@ -10,58 +10,48 @@ // about state in an instruction are correct before implementing any logic. // They are meant only to be called in dev mode as sanity checks. -/** - * Stringifies values such that strings are wrapped in explicit quotation marks and - * other types are stringified normally. Used in error messages (e.g. assertThrow) - * to make it clear that certain values are of the string type when comparing. - * - * e.g. `expected "3" to be 3` is easier to understand than `expected 3 to be 3`. - * - * @param value The value to be stringified - * @returns The stringified value - */ -function stringifyValueForError(value: any): string { - if (value && value.native && value.native.outerHTML) { - return value.native.outerHTML; +export function assertNumber(actual: any, msg: string) { + if (typeof actual != 'number') { + throwError(msg); } - return typeof value === 'string' ? `"${value}"` : value; } -export function assertNumber(actual: any, name: string) { - (typeof actual != 'number') && assertThrow(actual, 'number', name, 'typeof =='); +export function assertEqual(actual: T, expected: T, msg: string) { + if (actual != expected) { + throwError(msg); + } } -export function assertEqual( - actual: T, expected: T, name: string, serializer?: ((v: T) => string)) { - (actual != expected) && assertThrow(actual, expected, name, '==', serializer); +export function assertNotEqual(actual: T, expected: T, msg: string) { + if (actual == expected) { + throwError(msg); + } } -export function assertLessThan(actual: T, expected: T, name: string) { - (actual >= expected) && assertThrow(actual, expected, name, '<'); +export function assertSame(actual: T, expected: T, msg: string) { + if (actual !== expected) { + throwError(msg); + } } -export function assertNotNull(actual: T, name: string) { - assertNotEqual(actual, null, name); +export function assertLessThan(actual: T, expected: T, msg: string) { + if (actual >= expected) { + throwError(msg); + } } -export function assertNotEqual(actual: T, expected: T, name: string) { - (actual == expected) && assertThrow(actual, expected, name, '!='); +export function assertNull(actual: T, msg: string) { + if (actual != null) { + throwError(msg); + } } -/** - * Throws an error with a message constructed from the arguments. - * - * @param actual The actual value (e.g. 3) - * @param expected The expected value (e.g. 5) - * @param name The name of the value being checked (e.g. attrs.length) - * @param operator The comparison operator (e.g. <, >, ==) - * @param serializer Function that maps a value to its display value - */ -export function assertThrow( - actual: T, expected: T, name: string, operator: string, - serializer: ((v: T) => string) = stringifyValueForError): never { - const error = - `ASSERT: expected ${name} ${operator} ${serializer(expected)} but was ${serializer(actual)}!`; - debugger; // leave `debugger` here to aid in debugging. - throw new Error(error); +export function assertNotNull(actual: T, msg: string) { + if (actual == null) { + throwError(msg); + } } + +function throwError(msg: string): never { + throw new Error(`ASSERTION ERROR: ${msg}`); +} \ No newline at end of file diff --git a/packages/core/src/render3/component.ts b/packages/core/src/render3/component.ts index 5d5a8d3318..fdb25d5196 100644 --- a/packages/core/src/render3/component.ts +++ b/packages/core/src/render3/component.ts @@ -189,12 +189,12 @@ export function renderComponent( } export function detectChanges(component: T) { - ngDevMode && assertNotNull(component, 'component'); + ngDevMode && assertNotNull(component, 'detectChanges should be called with a component'); const hostNode = (component as any)[NG_HOST_SYMBOL] as LElementNode; if (ngDevMode && !hostNode) { createError('Not a directive instance', component); } - ngDevMode && assertNotNull(hostNode.data, 'hostNode.data'); + ngDevMode && assertNotNull(hostNode.data, 'Component host node should be attached to an LView'); renderComponentOrTemplate(hostNode, hostNode.view, component); isDirty = false; } @@ -202,7 +202,7 @@ export function detectChanges(component: T) { let isDirty = false; export function markDirty( component: T, scheduler: (fn: () => void) => void = requestAnimationFrame) { - ngDevMode && assertNotNull(component, 'component'); + ngDevMode && assertNotNull(component, 'markDirty should be called with a component'); if (!isDirty) { isDirty = true; scheduler(() => detectChanges(component)); diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index f6bc468888..fa8fcfdc05 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -8,7 +8,7 @@ import './ng_dev_mode'; -import {assertEqual, assertLessThan, assertNotEqual, assertNotNull} from './assert'; +import {assertEqual, assertLessThan, assertNotEqual, assertNotNull, assertNull, assertSame} from './assert'; import {LContainer, TContainer} from './interfaces/container'; import {CssSelector, LProjection} from './interfaces/projection'; import {LQueries} from './interfaces/query'; @@ -233,7 +233,7 @@ export function createLNode( if ((type & LNodeFlags.ViewOrElement) === LNodeFlags.ViewOrElement && isState) { // Bit of a hack to bust through the readonly because there is a circular dep between // LView and LNode. - ngDevMode && assertEqual((state as LView).node, null, 'lView.node'); + ngDevMode && assertNull((state as LView).node, 'LView.node should not have been initialized'); (state as LView as{node: LNode}).node = node; } if (index != null) { @@ -254,13 +254,17 @@ export function createLNode( if (previousOrParentNode.view === currentView || (previousOrParentNode.flags & LNodeFlags.TYPE_MASK) === LNodeFlags.View) { // We are in the same view, which means we are adding content node to the parent View. - ngDevMode && assertEqual(previousOrParentNode.child, null, 'previousNode.child'); + ngDevMode && assertNull( + previousOrParentNode.child, + `previousOrParentNode's child should not have been set.`); previousOrParentNode.child = node; } else { // We are adding component view, so we don't link parent node child to this node. } } else if (previousOrParentNode) { - ngDevMode && assertEqual(previousOrParentNode.next, null, 'previousNode.next'); + ngDevMode && assertNull( + previousOrParentNode.next, + `previousOrParentNode's next property should not have been set.`); previousOrParentNode.next = node; } } @@ -300,7 +304,7 @@ export function renderTemplate( -1, providedRendererFactory.createRenderer(null, null), getOrCreateTView(template))); } const hostView = host.data !; - ngDevMode && assertNotEqual(hostView, null, 'hostView'); + ngDevMode && assertNotNull(hostView, 'Host node should have an LView defined in host.data.'); renderComponentOrTemplate(host, hostView, context, template); return host; } @@ -381,7 +385,8 @@ export function elementStart( const node = data[index] !; native = node && (node as LElementNode).native; } else { - ngDevMode && assertEqual(currentView.bindingStartIndex, null, 'bindingStartIndex'); + ngDevMode && + assertNull(currentView.bindingStartIndex, 'elements should be created before any bindings'); const isHostElement = typeof nameOrComponentType !== 'string'; // MEGAMORPHIC: `ngComponentDef` is a megamorphic property access here. // This is OK, since we will refactor this code and store the result in `TView.data` @@ -504,7 +509,7 @@ export function createTView(): TView { } function setUpAttributes(native: RElement, attrs: string[]): void { - ngDevMode && assertEqual(attrs.length % 2, 0, 'attrs.length % 2'); + ngDevMode && assertEqual(attrs.length % 2, 0, 'each attribute should have a key and a value'); const isProc = isProceduralRenderer(renderer); for (let i = 0; i < attrs.length; i += 2) { @@ -809,7 +814,8 @@ export function elementStyle( * If value is not provided than the actual creation of the text node is delayed. */ export function text(index: number, value?: any): void { - ngDevMode && assertEqual(currentView.bindingStartIndex, null, 'bindingStartIndex'); + ngDevMode && + assertNull(currentView.bindingStartIndex, 'text nodes should be created before bindings'); const textNode = value != null ? (isProceduralRenderer(renderer) ? renderer.createText(stringify(value)) : renderer.createTextNode(stringify(value))) : @@ -865,7 +871,8 @@ export function textBinding(index: number, value: T | NO_CHANGE): void { export function directiveCreate( index: number, directive: T, directiveDef: DirectiveDef, queryName?: string | null): T { let instance; - ngDevMode && assertEqual(currentView.bindingStartIndex, null, 'bindingStartIndex'); + ngDevMode && + assertNull(currentView.bindingStartIndex, 'directives should be created before any bindings'); ngDevMode && assertPreviousIsParent(); let flags = previousOrParentNode !.flags; let size = flags & LNodeFlags.SIZE_MASK; @@ -984,10 +991,12 @@ function generateInitialInputs( export function container( index: number, directiveTypes?: DirectiveType[], template?: ComponentTemplate, tagName?: string, attrs?: string[], localRefs?: string[] | null): void { - ngDevMode && assertEqual(currentView.bindingStartIndex, null, 'bindingStartIndex'); + ngDevMode && + assertNull( + currentView.bindingStartIndex, 'container nodes should be created before any bindings'); const currentParent = isParent ? previousOrParentNode : previousOrParentNode.parent !; - ngDevMode && assertNotEqual(currentParent, null, 'currentParent'); + ngDevMode && assertNotNull(currentParent, 'containers should have a parent'); const lContainer = { views: [], @@ -1037,9 +1046,9 @@ export function containerRefreshStart(index: number): void { ngDevMode && assertNodeType(previousOrParentNode, LNodeFlags.Container); isParent = true; (previousOrParentNode as LContainerNode).data.nextIndex = 0; - ngDevMode && assertEqual( - (previousOrParentNode as LContainerNode).native === undefined, true, - 'previousOrParentNode.native === undefined'); + ngDevMode && assertSame( + (previousOrParentNode as LContainerNode).native, undefined, + `the container's native element should not have been set yet.`); // We need to execute init hooks here so ngOnInit hooks are called in top level views // before they are called in embedded views (for backwards compatibility). @@ -1181,11 +1190,11 @@ export function componentRefresh(directiveIndex: number, elementIndex: number ngDevMode && assertDataInRange(elementIndex); const element = data ![elementIndex] as LElementNode; ngDevMode && assertNodeType(element, LNodeFlags.Element); - ngDevMode && assertNotEqual(element.data, null, 'isComponent'); + ngDevMode && + assertNotNull(element.data, `Component's host node should have an LView attached.`); ngDevMode && assertDataInRange(directiveIndex); const directive = getDirectiveInstance(data[directiveIndex]); const hostView = element.data !; - ngDevMode && assertNotEqual(hostView, null, 'hostView'); const oldView = enterView(hostView, element); try { template(directive, creationMode); @@ -1243,9 +1252,9 @@ function appendToProjectionNode( projectionNode: LProjectionNode, appendedFirst: LElementNode | LTextNode | LContainerNode | null, appendedLast: LElementNode | LTextNode | LContainerNode | null) { - // appendedFirst can be null if and only if appendedLast is also null - ngDevMode && - assertEqual(!appendedFirst === !appendedLast, true, '!appendedFirst === !appendedLast'); + ngDevMode && assertEqual( + !!appendedFirst, !!appendedLast, + 'appendedFirst can be null if and only if appendedLast is also null'); if (!appendedLast) { // nothing to append return; @@ -1760,18 +1769,18 @@ export function getDirectiveInstance(instanceOrArray: T | [T]): T { } export function assertPreviousIsParent() { - assertEqual(isParent, true, 'isParent'); + assertEqual(isParent, true, 'previousOrParentNode should be a parent'); } function assertHasParent() { - assertNotEqual(previousOrParentNode.parent, null, 'isParent'); + assertNotNull(previousOrParentNode.parent, 'previousOrParentNode should have a parent'); } function assertDataInRange(index: number, arr?: any[]) { if (arr == null) arr = data; - assertLessThan(index, arr ? arr.length : 0, 'data.length'); + assertLessThan(index, arr ? arr.length : 0, 'index expected to be a valid data index'); } function assertDataNext(index: number) { - assertEqual(data.length, index, 'data.length not in sequence'); + assertEqual(data.length, index, 'index expected to be at the end of data'); } diff --git a/packages/core/src/render3/node_assert.ts b/packages/core/src/render3/node_assert.ts index 15b82f9297..b73cc06f51 100644 --- a/packages/core/src/render3/node_assert.ts +++ b/packages/core/src/render3/node_assert.ts @@ -6,30 +6,25 @@ * found in the LICENSE file at https://angular.io/license */ -import {assertEqual, assertNotEqual} from './assert'; +import {assertEqual, assertNotNull} from './assert'; import {LNode, LNodeFlags} from './interfaces/node'; export function assertNodeType(node: LNode, type: LNodeFlags) { - assertNotEqual(node, null, 'node'); - assertEqual(node.flags & LNodeFlags.TYPE_MASK, type, 'Node.type', typeSerializer); + assertNotNull(node, 'should be called with a node'); + assertEqual(node.flags & LNodeFlags.TYPE_MASK, type, `should be a ${typeName(type)}`); } export function assertNodeOfPossibleTypes(node: LNode, ...types: LNodeFlags[]) { - assertNotEqual(node, null, 'node'); - const nodeType = (node.flags & LNodeFlags.TYPE_MASK); - for (let i = 0; i < types.length; i++) { - if (nodeType === types[i]) { - return; - } - } - throw new Error( - `Expected node of possible types: ${types.map(typeSerializer).join(', ')} but got ${typeSerializer(nodeType)}`); + assertNotNull(node, 'should be called with a node'); + const nodeType = node.flags & LNodeFlags.TYPE_MASK; + const found = types.some(type => nodeType === type); + assertEqual(found, true, `Should be one of ${types.map(typeName).join(', ')}`); } -function typeSerializer(type: LNodeFlags): string { +function typeName(type: LNodeFlags): string { if (type == LNodeFlags.Projection) return 'Projection'; if (type == LNodeFlags.Container) return 'Container'; if (type == LNodeFlags.View) return 'View'; if (type == LNodeFlags.Element) return 'Element'; - return '??? ' + type + ' ???'; + return ''; } diff --git a/packages/core/src/render3/node_selector_matcher.ts b/packages/core/src/render3/node_selector_matcher.ts index ce83368f69..b1a508d85f 100644 --- a/packages/core/src/render3/node_selector_matcher.ts +++ b/packages/core/src/render3/node_selector_matcher.ts @@ -37,7 +37,7 @@ function isCssClassMatching(nodeClassAttrVal: string, cssClassToMatch: string): */ export function isNodeMatchingSimpleSelector(tNode: TNode, selector: SimpleCssSelector): boolean { const noOfSelectorParts = selector.length; - ngDevMode && assertNotNull(selector[0], 'selector[0]'); + ngDevMode && assertNotNull(selector[0], 'the selector should have a tag name'); const tagNameInSelector = selector[0]; // check tag tame diff --git a/packages/core/src/render3/query.ts b/packages/core/src/render3/query.ts index 1d293db2db..63afa798db 100644 --- a/packages/core/src/render3/query.ts +++ b/packages/core/src/render3/query.ts @@ -245,7 +245,7 @@ function add(query: LQuery| null, node: LNode) { if (directiveIdx !== null) { // a node is matching a predicate - determine what to read // note that queries using name selector must specify read strategy - ngDevMode && assertNotNull(predicate.read, 'predicate.read'); + ngDevMode && assertNotNull(predicate.read, 'the node should have a predicate'); const result = readFromNodeInjector(nodeInjector, node, predicate.read !, directiveIdx); if (result !== null) { addMatch(query, result);