diff --git a/src/core/application.ts b/src/core/application.ts index b8b92715..c9f9548d 100644 --- a/src/core/application.ts +++ b/src/core/application.ts @@ -6,8 +6,9 @@ import { Logger } from "./logger" import { Router } from "./router" import { Schema, defaultSchema } from "./schema" import { ActionDescriptorFilter, ActionDescriptorFilters, defaultActionDescriptorFilters } from "./action_descriptor" +import { WarningHandler } from "./warning_handler" -export class Application implements ErrorHandler { +export class Application implements ErrorHandler, WarningHandler { readonly element: Element readonly schema: Schema readonly dispatcher: Dispatcher @@ -15,6 +16,7 @@ export class Application implements ErrorHandler { readonly actionDescriptorFilters: ActionDescriptorFilters logger: Logger = console debug = false + warnings = true static start(element?: Element, schema?: Schema): Application { const application = new this(element, schema) @@ -90,7 +92,13 @@ export class Application implements ErrorHandler { window.onerror?.(message, "", 0, 0, error) } - // Debug logging + // Logging + + handleWarning(warning: string, message: string, detail: any) { + if (this.warnings) { + this.logger.warn(`%s\n\n%s\n\n%o`, message, warning, detail) + } + } logDebugActivity = (identifier: string, functionName: string, detail: object = {}): void => { if (this.debug) { diff --git a/src/core/binding_observer.ts b/src/core/binding_observer.ts index 62cc8355..fc0ecfa3 100644 --- a/src/core/binding_observer.ts +++ b/src/core/binding_observer.ts @@ -61,6 +61,20 @@ export class BindingObserver implements ValueListObserverDelegate { const binding = new Binding(this.context, action) this.bindingsByAction.set(action, binding) this.delegate.bindingConnected(binding) + const { controller } = binding.context + const method = (controller as any)[action.methodName] + + if (typeof method !== "function") { + this.context.handleWarning( + `Stimulus is unable to connect the action "${action.methodName}" with an action on the controller "${action.identifier}". Please make sure the action references a valid method on the controller.`, + `Element references undefined action "${action.methodName}" on controller with identifier "${action.identifier}"`, + { + element: action.element, + identifier: action.identifier, + methodName: action.methodName, + } + ) + } } private disconnectAction(action: Action) { @@ -92,4 +106,14 @@ export class BindingObserver implements ValueListObserverDelegate { elementUnmatchedValue(element: Element, action: Action) { this.disconnectAction(action) } + + elementMatchedNoValue(element: Element, token: Token, error?: Error) { + const { content: action } = token + + if (error) { + this.context.handleWarning(`Warning connecting action "${action}"`, error.message, { action, element }) + } else { + this.context.handleElementMatchedNoValue(element, token, error) + } + } } diff --git a/src/core/context.ts b/src/core/context.ts index e1187add..8a61ddb6 100644 --- a/src/core/context.ts +++ b/src/core/context.ts @@ -10,6 +10,7 @@ import { ValueObserver } from "./value_observer" import { TargetObserver, TargetObserverDelegate } from "./target_observer" import { OutletObserver, OutletObserverDelegate } from "./outlet_observer" import { namespaceCamelize } from "./string_helpers" +import { Token } from "../mutation-observers" export class Context implements ErrorHandler, TargetObserverDelegate, OutletObserverDelegate { readonly module: Module @@ -101,7 +102,11 @@ export class Context implements ErrorHandler, TargetObserverDelegate, OutletObse this.application.handleError(error, `Error ${message}`, detail) } - // Debug logging + // Logging + + handleWarning(warning: string, message: string, detail: object = {}) { + this.application.handleWarning(warning, `Warning: ${message}`, detail) + } logDebugActivity = (functionName: string, detail: object = {}): void => { const { identifier, controller, element } = this @@ -137,4 +142,8 @@ export class Context implements ErrorHandler, TargetObserverDelegate, OutletObse controller[methodName](...args) } } + + handleElementMatchedNoValue(element: Element, token: Token, error?: Error) { + this.application.router.handleUniqueWarningsForUnregisteredControllers(element, token, error) + } } diff --git a/src/core/guide.ts b/src/core/guide.ts index e96a2293..3e9cf0df 100644 --- a/src/core/guide.ts +++ b/src/core/guide.ts @@ -1,14 +1,14 @@ -import { Logger } from "./logger" +import { WarningHandler } from "./warning_handler" export class Guide { - readonly logger: Logger + readonly warningHandler: WarningHandler readonly warnedKeysByObject: WeakMap> = new WeakMap() - constructor(logger: Logger) { - this.logger = logger + constructor(warningHandler: WarningHandler) { + this.warningHandler = warningHandler } - warn(object: any, key: string, message: string) { + warn(object: any, key: string, warning: string, message: string, detail?: object) { let warnedKeys: Set | undefined = this.warnedKeysByObject.get(object) if (!warnedKeys) { @@ -18,7 +18,7 @@ export class Guide { if (!warnedKeys.has(key)) { warnedKeys.add(key) - this.logger.warn(message, object) + this.warningHandler.handleWarning(warning, message, detail || object) } } } diff --git a/src/core/router.ts b/src/core/router.ts index 6b63318d..bafca88e 100644 --- a/src/core/router.ts +++ b/src/core/router.ts @@ -5,18 +5,23 @@ import { Module } from "./module" import { Multimap } from "../multimap" import { Scope } from "./scope" import { ScopeObserver, ScopeObserverDelegate } from "./scope_observer" +import { Token } from "../mutation-observers" +import { Guide } from "./guide" +import { Action } from "./action" export class Router implements ScopeObserverDelegate { readonly application: Application private scopeObserver: ScopeObserver private scopesByIdentifier: Multimap private modulesByIdentifier: Map + private guide: Guide constructor(application: Application) { this.application = application this.scopeObserver = new ScopeObserver(this.element, this.schema, this) this.scopesByIdentifier = new Multimap() this.modulesByIdentifier = new Map() + this.guide = new Guide(this.warningHandler) } get element() { @@ -27,8 +32,8 @@ export class Router implements ScopeObserverDelegate { return this.application.schema } - get logger() { - return this.application.logger + get warningHandler() { + return this.application } get controllerAttribute(): string { @@ -81,17 +86,44 @@ export class Router implements ScopeObserverDelegate { this.application.handleError(error, message, detail) } + handleUniqueWarningsForUnregisteredControllers(element: Element, token: Token, error?: Error) { + if (!error) { + const { identifier } = Action.forToken(token, this.schema) + const registeredIdentifiers = Array.from(this.modulesByIdentifier.keys()).sort() + + if (!registeredIdentifiers.includes(identifier)) { + this.guide.warn( + element, + `identifier:${identifier}`, + `Stimulus is unable to find a registered controller with identifier "${identifier}" that is referenced from action "${token.content}". The specified controller is not registered within the application. Please ensure that the controller with the identifier "${identifier}" is properly registered. For reference, the warning details include a list of all currently registered controller identifiers.`, + `Warning: Element references unknown action for non-registered controller "${identifier}"`, + { identifier, element, registeredIdentifiers } + ) + } + } + } + // Scope observer delegate createScopeForElementAndIdentifier(element: Element, identifier: string) { - return new Scope(this.schema, element, identifier, this.logger) + return new Scope(this.schema, element, identifier, this.application) } scopeConnected(scope: Scope) { - this.scopesByIdentifier.add(scope.identifier, scope) - const module = this.modulesByIdentifier.get(scope.identifier) + const { element, identifier } = scope + this.scopesByIdentifier.add(identifier, scope) + const module = this.modulesByIdentifier.get(identifier) + if (module) { module.connectContextForScope(scope) + } else { + const registeredIdentifiers = Array.from(this.modulesByIdentifier.keys()).sort() + + this.application.handleWarning( + `Stimulus is unable to connect the controller with identifier "${identifier}". The specified controller is not registered within the application. Please ensure that the controller with the identifier "${identifier}" is properly registered. For reference, the warning details include a list of all currently registered controller identifiers.`, + `Warning: Element references an unregistered controller identifier "${identifier}".`, + { identifier, element, registeredIdentifiers } + ) } } diff --git a/src/core/scope.ts b/src/core/scope.ts index de302807..6bd54f40 100644 --- a/src/core/scope.ts +++ b/src/core/scope.ts @@ -1,11 +1,11 @@ import { ClassMap } from "./class_map" import { DataMap } from "./data_map" import { Guide } from "./guide" -import { Logger } from "./logger" import { Schema } from "./schema" import { attributeValueContainsToken } from "./selectors" import { TargetSet } from "./target_set" import { OutletSet } from "./outlet_set" +import { WarningHandler } from "./warning_handler" export class Scope { readonly schema: Schema @@ -17,11 +17,11 @@ export class Scope { readonly classes = new ClassMap(this) readonly data = new DataMap(this) - constructor(schema: Schema, element: Element, identifier: string, logger: Logger) { + constructor(schema: Schema, element: Element, identifier: string, warningHandler: WarningHandler) { this.schema = schema this.element = element this.identifier = identifier - this.guide = new Guide(logger) + this.guide = new Guide(warningHandler) this.outlets = new OutletSet(this.documentScope, element) } @@ -55,6 +55,6 @@ export class Scope { private get documentScope(): Scope { return this.isDocumentScope ? this - : new Scope(this.schema, document.documentElement, this.identifier, this.guide.logger) + : new Scope(this.schema, document.documentElement, this.identifier, this.guide.warningHandler) } } diff --git a/src/core/scope_observer.ts b/src/core/scope_observer.ts index 6ddcd0ef..e04de84c 100644 --- a/src/core/scope_observer.ts +++ b/src/core/scope_observer.ts @@ -79,4 +79,6 @@ export class ScopeObserver implements ValueListObserverDelegate { } return scopesByIdentifier } + + elementMatchedNoValue() {} } diff --git a/src/core/target_set.ts b/src/core/target_set.ts index bd83e691..74d5d77e 100644 --- a/src/core/target_set.ts +++ b/src/core/target_set.ts @@ -80,6 +80,7 @@ export class TargetSet { this.guide.warn( element, `target:${targetName}`, + `Warning: Deprecated attributeName "${attributeName}"`, `Please replace ${attributeName}="${identifier}.${targetName}" with ${revisedAttributeName}="${targetName}". ` + `The ${attributeName} attribute is deprecated and will be removed in a future version of Stimulus.` ) diff --git a/src/core/warning_handler.ts b/src/core/warning_handler.ts new file mode 100644 index 00000000..f2e83483 --- /dev/null +++ b/src/core/warning_handler.ts @@ -0,0 +1,3 @@ +export interface WarningHandler { + handleWarning(warning: string, message: string, detail: any): void +} diff --git a/src/mutation-observers/value_list_observer.ts b/src/mutation-observers/value_list_observer.ts index 35cd214a..e5b7e412 100644 --- a/src/mutation-observers/value_list_observer.ts +++ b/src/mutation-observers/value_list_observer.ts @@ -3,6 +3,7 @@ import { Token, TokenListObserver, TokenListObserverDelegate } from "./token_lis export interface ValueListObserverDelegate { parseValueForToken(token: Token): T | undefined elementMatchedValue(element: Element, value: T): void + elementMatchedNoValue(element: Element, token: Token, error?: Error): void elementUnmatchedValue(element: Element, value: T): void } @@ -50,10 +51,12 @@ export class ValueListObserver implements TokenListObserverDelegate { tokenMatched(token: Token) { const { element } = token - const { value } = this.fetchParseResultForToken(token) + const { error, value } = this.fetchParseResultForToken(token) if (value) { this.fetchValuesByTokenForElement(element).set(token, value) this.delegate.elementMatchedValue(element, value) + } else { + this.delegate.elementMatchedNoValue(element, token, error) } } diff --git a/src/tests/cases/application_test_case.ts b/src/tests/cases/application_test_case.ts index 14b01750..45125cd6 100644 --- a/src/tests/cases/application_test_case.ts +++ b/src/tests/cases/application_test_case.ts @@ -15,6 +15,7 @@ export class ApplicationTestCase extends DOMTestCase { async runTest(testName: string) { try { this.application = new TestApplication(this.fixtureElement, this.schema) + this.application.warnings = false this.setupApplication() this.application.start() await super.runTest(testName) diff --git a/src/tests/modules/core/error_handler_tests.ts b/src/tests/modules/core/error_handler_tests.ts index d94482ea..4be58900 100644 --- a/src/tests/modules/core/error_handler_tests.ts +++ b/src/tests/modules/core/error_handler_tests.ts @@ -2,7 +2,7 @@ import { Controller } from "../../../core/controller" import { Application } from "../../../core/application" import { ControllerTestCase } from "../../cases/controller_test_case" -class MockLogger { +export class MockLogger { errors: any[] = [] logs: any[] = [] warns: any[] = [] @@ -15,8 +15,8 @@ class MockLogger { this.errors.push(event) } - warn(event: any) { - this.warns.push(event) + warn(event: any, message: string, warning: string, detail: string) { + this.warns.push({ message, warning, detail }) } groupCollapsed() {} diff --git a/src/tests/modules/core/legacy_target_tests.ts b/src/tests/modules/core/legacy_target_tests.ts index 331035cc..6ec0bd6b 100644 --- a/src/tests/modules/core/legacy_target_tests.ts +++ b/src/tests/modules/core/legacy_target_tests.ts @@ -20,6 +20,7 @@ export default class LegacyTargetTests extends ControllerTestCase(TargetControll async setupApplication() { super.setupApplication() + this.application.warnings = true this.application.logger = Object.create(console, { warn: { value: () => this.warningCount++, diff --git a/src/tests/modules/core/warning_tests.ts b/src/tests/modules/core/warning_tests.ts new file mode 100644 index 00000000..6ed8e703 --- /dev/null +++ b/src/tests/modules/core/warning_tests.ts @@ -0,0 +1,117 @@ +import { ApplicationTestCase } from "../../cases" +import { Controller } from "../../../core" +import { MockLogger } from "./error_handler_tests" + +class WarningController extends Controller { + found() {} +} + +export default class WarningTests extends ApplicationTestCase { + async setupApplication() { + const logger = new MockLogger() + + this.application.warnings = true + this.application.logger = logger + + const identifiers: string[] = ["controller1", "controller2"] + + identifiers.forEach((identifier) => { + this.application.register(identifier, WarningController) + }) + } + + get mockLogger(): MockLogger { + return this.application.logger as any + } + + async "test warnings for undefined controllers"() { + await this.setAttribute(this.fixtureElement, "data-controller", "controller1 unknown") + + this.assert.equal(this.mockLogger.warns.length, 1) + + const { message, warning, detail } = this.mockLogger.warns[0] + + this.assert.equal(message, 'Warning: Element references an unregistered controller identifier "unknown".') + this.assert.equal( + warning, + 'Stimulus is unable to connect the controller with identifier "unknown". The specified controller is not registered within the application. Please ensure that the controller with the identifier "unknown" is properly registered. For reference, the warning details include a list of all currently registered controller identifiers.' + ) + this.assert.deepEqual(detail, { + element: this.fixtureElement, + identifier: "unknown", + registeredIdentifiers: ["controller1", "controller2"], + }) + } + + async "test no warnings for found controllers"() { + await this.setAttribute(this.fixtureElement, "data-controller", "controller1 controller2") + + this.assert.equal(this.mockLogger.warns.length, 0) + } + + async "test warnings for undefined actions"() { + await this.renderFixture( + `` + ) + + this.assert.equal(this.mockLogger.warns.length, 1) + + const { message, warning, detail } = this.mockLogger.warns[0] + + this.assert.equal( + message, + 'Warning: Element references undefined action "not-found" on controller with identifier "controller2"' + ) + this.assert.equal( + warning, + 'Stimulus is unable to connect the action "not-found" with an action on the controller "controller2". Please make sure the action references a valid method on the controller.' + ) + this.assert.deepEqual(detail, { + element: this.fixtureElement.children[0], + identifier: "controller2", + methodName: "not-found", + }) + } + + async "test errors from elementMatchedNoValue being forwarded as warnings"() { + await this.renderFixture(`
`) + + this.assert.equal(this.mockLogger.warns.length, 1) + this.assert.equal(this.mockLogger.warns[0].message, "Warning: missing event name") + this.assert.equal(this.mockLogger.warns[0].warning, 'Warning connecting action "controller2#not-a-method"') + } + + async "test unique warnings for actions referencing undefined controllers"() { + await this.renderFixture( + `` + ) + + this.assert.equal(this.mockLogger.warns.length, 2) + + this.assert.deepEqual( + this.mockLogger.warns + .map((warn) => ({ message: warn.message, warning: warn.warning })) + .sort((warn) => warn.warning), + [ + { + message: 'Warning: Element references unknown action for non-registered controller "non-existing-controller"', + warning: + 'Stimulus is unable to find a registered controller with identifier "non-existing-controller" that is referenced from action "non-existing-controller#found". The specified controller is not registered within the application. Please ensure that the controller with the identifier "non-existing-controller" is properly registered. For reference, the warning details include a list of all currently registered controller identifiers.', + }, + { + message: 'Warning: Element references unknown action for non-registered controller "non-existing-controller"', + warning: + 'Stimulus is unable to find a registered controller with identifier "non-existing-controller" that is referenced from action "non-existing-controller#not-found". The specified controller is not registered within the application. Please ensure that the controller with the identifier "non-existing-controller" is properly registered. For reference, the warning details include a list of all currently registered controller identifiers.', + }, + ] + ) + } + + async "test no warnings for found actions"() { + await this.renderFixture( + `` + ) + + this.assert.equal(this.mockLogger.warns.length, 0) + } +} diff --git a/src/tests/modules/mutation-observers/value_list_observer_tests.ts b/src/tests/modules/mutation-observers/value_list_observer_tests.ts index 1a4b1517..cb457d50 100644 --- a/src/tests/modules/mutation-observers/value_list_observer_tests.ts +++ b/src/tests/modules/mutation-observers/value_list_observer_tests.ts @@ -79,6 +79,23 @@ export default class ValueListObserverTests extends ObserverTestCase implements ]) } + async "test forwards errors to elementMatchedNoValue"() { + this.valueString = "unknown" + await this.nextFrame + + this.assert.deepEqual(this.testCalls, [ + ["elementUnmatchedValue", this.element, 1, "one"], + ["elementMatchedNoValue", this.element, "unknown", "unknown token throws error"], + ]) + } + + async "test doesnt call elementMatchedNoValue when parseValueForToken returns undefined"() { + this.valueString = "undefined" + await this.nextFrame + + this.assert.deepEqual(this.testCalls, [["elementUnmatchedValue", this.element, 1, "one"]]) + } + get element() { return this.findElement("div") } @@ -90,6 +107,14 @@ export default class ValueListObserverTests extends ObserverTestCase implements // Value observer delegate parseValueForToken(token: Token) { + if (token.content === "unknown") { + throw new Error("unknown token throws error") + } + + if (token.content === "undefined") { + return undefined + } + return { id: ++this.lastValueId, token } } @@ -100,4 +125,8 @@ export default class ValueListObserverTests extends ObserverTestCase implements elementUnmatchedValue(element: Element, value: Value) { this.recordCall("elementUnmatchedValue", element, value.id, value.token.content) } + + elementMatchedNoValue(element: Element, token: Token, error: Error) { + this.recordCall("elementMatchedNoValue", element, token.content, error.message) + } }