Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handleWarning on undefined controller or action #653

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
6ededb8
warning on undefined controller or action
AndersGM Feb 6, 2023
6e5577c
Clean up
AndersGM Feb 14, 2023
fd05405
fixed offenses
AndersGM Feb 16, 2023
3008786
Merge remote-tracking branch 'origin/main' into warn-on-missing-ident…
AndersGM Feb 16, 2023
d262388
forward error
AndersGM Feb 27, 2023
82527c1
added spec
AndersGM Feb 27, 2023
5717328
fixed offenses
AndersGM Feb 27, 2023
1051451
Merge remote-tracking branch 'origin/main' into warn-on-missing-ident…
AndersGM Feb 27, 2023
8583637
removed unused args
AndersGM Feb 27, 2023
832872c
better test name
AndersGM Feb 27, 2023
a528f06
Merge remote-tracking branch 'origin' into warn-on-missing-identifier
AndersGM May 20, 2023
b33b671
added better tests
AndersGM Jun 15, 2023
b58fa98
clean up specs
AndersGM Jun 15, 2023
0107294
finished last test case in a non pretty way
AndersGM Jun 16, 2023
d656869
Update warning message for unregistered Stimulus controller
marcoroth Jun 15, 2023
35c7e69
updated specs after improved error message
AndersGM Jun 16, 2023
e710672
fixed offenses
AndersGM Jun 16, 2023
e43fdc1
refactored to warningHandler and use appropriatly
AndersGM Jun 16, 2023
bd09b5a
Merge remote-tracking branch 'origin/main' into warn-on-missing-ident…
AndersGM Jun 16, 2023
d3228ea
import mocklogger from error handler
AndersGM Jun 16, 2023
b1b67c7
removed duplicated test
AndersGM Jun 16, 2023
da05e5e
improved messages
AndersGM Jun 16, 2023
9efadc0
include warning in message
AndersGM Jun 17, 2023
c61ee11
test errors being forwarded
AndersGM Jun 17, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/core/application.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,17 @@ 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
readonly router: Router
readonly actionDescriptorFilters: ActionDescriptorFilters
logger: Logger = console
debug = false
warnings = true

static start(element?: Element, schema?: Schema): Application {
const application = new this(element, schema)
Expand Down Expand Up @@ -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) {
Expand Down
24 changes: 24 additions & 0 deletions src/core/binding_observer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,20 @@ export class BindingObserver implements ValueListObserverDelegate<Action> {
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) {
Expand Down Expand Up @@ -92,4 +106,14 @@ export class BindingObserver implements ValueListObserverDelegate<Action> {
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)
}
}
}
11 changes: 10 additions & 1 deletion src/core/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}
12 changes: 6 additions & 6 deletions src/core/guide.ts
Original file line number Diff line number Diff line change
@@ -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<any, Set<string>> = 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<string> | undefined = this.warnedKeysByObject.get(object)

if (!warnedKeys) {
Expand All @@ -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)
}
}
}
42 changes: 37 additions & 5 deletions src/core/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, Scope>
private modulesByIdentifier: Map<string, Module>
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() {
Expand All @@ -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 {
Expand Down Expand Up @@ -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 }
)
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/core/scope.ts
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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)
}

Expand Down Expand Up @@ -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)
}
}
2 changes: 2 additions & 0 deletions src/core/scope_observer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,6 @@ export class ScopeObserver implements ValueListObserverDelegate<Scope> {
}
return scopesByIdentifier
}

elementMatchedNoValue() {}
}
1 change: 1 addition & 0 deletions src/core/target_set.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.`
)
Expand Down
3 changes: 3 additions & 0 deletions src/core/warning_handler.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export interface WarningHandler {
handleWarning(warning: string, message: string, detail: any): void
}
5 changes: 4 additions & 1 deletion src/mutation-observers/value_list_observer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Token, TokenListObserver, TokenListObserverDelegate } from "./token_lis
export interface ValueListObserverDelegate<T> {
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
}

Expand Down Expand Up @@ -50,10 +51,12 @@ export class ValueListObserver<T> 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)
}
}

Expand Down
1 change: 1 addition & 0 deletions src/tests/cases/application_test_case.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions src/tests/modules/core/error_handler_tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = []
Expand All @@ -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() {}
Expand Down
1 change: 1 addition & 0 deletions src/tests/modules/core/legacy_target_tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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++,
Expand Down
Loading