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

Emit warnings for undefined controllers, actions and targets #413

Merged
merged 5 commits into from
Jun 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions packages/@stimulus/core/src/application.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export class Application implements ErrorHandler {
readonly router: Router
logger: Logger = console
debug: boolean = false
warnings: boolean = true

static start(element?: Element, schema?: Schema): Application {
const application = new Application(element, schema)
Expand Down Expand Up @@ -71,6 +72,14 @@ export class Application implements ErrorHandler {
return context ? context.controller : null
}

// Warning handling

handleWarning(warning: string, message: string, detail: object) {
if (this.warnings) {
this.logger.warn(`%s\n\n%s\n\n%o`, message, warning, detail)
}
}

// Error handling

handleError(error: Error, message: string, detail: object) {
Expand Down
18 changes: 18 additions & 0 deletions packages/@stimulus/core/src/binding_observer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,18 @@ export class BindingObserver implements ValueListObserverDelegate<Action> {

private connectAction(action: Action) {
const binding = new Binding(this.context, action)
const { controller } = binding.context
const method = (controller as any)[action.methodName]

this.bindingsByAction.set(action, binding)
this.delegate.bindingConnected(binding)

if (typeof method != "function") {
this.context.handleWarning(
`Action "${action.toString()}" references undefined method "${action.methodName}" on controller "${action.identifier}"`,
`connecting action "${action.toString()}"`
)
}
}

private disconnectAction(action: Action) {
Expand Down Expand Up @@ -92,4 +102,12 @@ export class BindingObserver implements ValueListObserverDelegate<Action> {
elementUnmatchedValue(element: Element, action: Action) {
this.disconnectAction(action)
}

elementMatchedNoValue(token: Token) {
const action = Action.forToken(token)
this.context.handleWarning(
`Action "${token.content}" references undefined controller "${action.identifier}"`,
`connecting action "${token.content}"`
)
}
}
11 changes: 10 additions & 1 deletion packages/@stimulus/core/src/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,22 @@ import { ErrorHandler } from "./error_handler"
import { Module } from "./module"
import { Schema } from "./schema"
import { Scope } from "./scope"
import { TargetGuide } from "./target_guide"
import { ValueObserver } from "./value_observer"

export class Context implements ErrorHandler {
readonly module: Module
readonly scope: Scope
readonly controller: Controller
readonly targetGuide: TargetGuide
private bindingObserver: BindingObserver
private valueObserver: ValueObserver

constructor(module: Module, scope: Scope) {
this.module = module
this.scope = scope
this.controller = new module.controllerConstructor(this)
this.targetGuide = new TargetGuide(this.scope, this.controller)
this.bindingObserver = new BindingObserver(this, this.dispatcher)
this.valueObserver = new ValueObserver(this, this.controller)

Expand Down Expand Up @@ -86,7 +89,13 @@ export class Context implements ErrorHandler {
this.application.handleError(error, `Error ${message}`, detail)
}

// Debug logging
// Logging

handleWarning(warning: string, message: string, detail: object = {}) {
const { identifier, controller, element } = this
detail = Object.assign({ identifier, controller, element }, detail)
this.application.handleWarning(warning, `Warning ${message}`, detail)
}

logDebugActivity = (functionName: string, detail: object = {}): void => {
const { identifier, controller, element } = this
Expand Down
12 changes: 10 additions & 2 deletions packages/@stimulus/core/src/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export class Router implements ScopeObserverDelegate {

unloadIdentifier(identifier: string) {
const module = this.modulesByIdentifier.get(identifier)

if (module) {
this.disconnectModule(module)
}
Expand All @@ -84,10 +85,17 @@ export class Router implements ScopeObserverDelegate {
}

scopeConnected(scope: Scope) {
this.scopesByIdentifier.add(scope.identifier, scope)
const module = this.modulesByIdentifier.get(scope.identifier)
const { identifier, element } = scope
this.scopesByIdentifier.add(identifier, scope)
const module = this.modulesByIdentifier.get(identifier)
if (module) {
module.connectContextForScope(scope)
} else {
this.application.handleWarning(
`Element references undefined controller "${identifier}"`,
`Warning connecting controller "${identifier}"`,
{ identifier, element }
)
}
}

Expand Down
2 changes: 2 additions & 0 deletions packages/@stimulus/core/src/scope_observer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ export class ScopeObserver implements ValueListObserverDelegate<Scope> {
}
}

elementMatchedNoValue(token: Token) {}

private fetchScopesByIdentifierForElement(element: Element) {
let scopesByIdentifier = this.scopesByIdentifierByElement.get(element)
if (!scopesByIdentifier) {
Expand Down
123 changes: 123 additions & 0 deletions packages/@stimulus/core/src/target_guide.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
import { Controller } from "./controller"
import { Constructor } from "./constructor"
import { Scope } from "./scope"
import { TargetDescriptor } from "./target_properties"
import { readInheritableStaticArrayValues } from "./inheritable_statics"

export class TargetGuide {
readonly scope: Scope
readonly controller: Controller
readonly definedTargets: Array<String>

constructor(scope: Scope, controller: Controller) {
this.scope = scope
this.controller = controller
this.definedTargets = readInheritableStaticArrayValues(this.controller.constructor as Constructor<Controller>, "targets")

this.searchForUndefinedTargets()
}

get identifier() {
return this.scope.identifier
}

get targets() {
return this.scope.targets
}

get registeredControllers(): Array<String> {
return this.controller.application.router.modules.map((c) => c.identifier)
}

controllerRegistered(controllerName: string): Boolean {
return this.registeredControllers.includes(controllerName)
}

targetDefined(targetName: string): Boolean {
return this.definedTargets.includes(targetName)
}

private getAllTargets(element: Element): Array<TargetDescriptor> {
const attribute = `data-${this.identifier}-target`
const selector = `[${attribute}]`

return Array.from(element.querySelectorAll(selector)).map((element: Element) => {
const target = element.getAttribute(attribute)
return {
identifier: this.identifier,
target,
element,
attribute,
legacy: false
}
})
}

private getAllLegacyTargets(element: Element): Array<TargetDescriptor> {
const attribute = "data-target"
const selector = `[${attribute}]`

return Array.from(element.querySelectorAll(selector)).map((element: Element) => {
const value = element.getAttribute(attribute)
const parts = value ? value.split(".") : []
return {
identifier: parts[0],
target: parts[1],
element,
attribute,
legacy: true
}
})
}

private searchForUndefinedTargets() {
const { element } = this.scope

const targets: Array<TargetDescriptor> = [
...this.getAllTargets(element),
...this.getAllLegacyTargets(element)
]

targets.forEach((descriptor) => {
const { identifier, attribute, target, legacy, element } = descriptor

if (identifier && target) {
this.handleWarningForUndefinedTarget(descriptor)
} else if (identifier && !target) {
this.controller.context.handleWarning(
`The "${attribute}" attribute of the Element doesn't include a target. Please specify a target for the "${identifier}" controller.`,
`connecting target for "${identifier}"`,
{ identifier, target, attribute, element }
)
} else if (legacy && !target) {
this.controller.context.handleWarning(
`The "${attribute}" attribute of the Element doesn't include a value. Please specify a controller and target value in the right format.`,
`connecting target`,
{ identifier, target, attribute, element }
)
}
})
}

private handleWarningForUndefinedTarget(descriptor: TargetDescriptor) {
const { identifier, target, element, attribute } = descriptor

if (identifier === this.identifier) {
if (!this.targetDefined(target as string)) {
this.controller.context.handleWarning(
`Element references undefined target "${target}" for controller "${identifier}". Make sure you defined the target "${target}" in the "static targets" array of your controller.`,
`connecting target "${identifier}.${target}"`,
{ identifier, target, element, attribute }
)
}
} else {
if (!this.controllerRegistered(identifier as string)) {
this.controller.context.handleWarning(
`Target "${target}" references undefined controller "${identifier}". Make sure you registered the "${identifier}" controller.`,
`connecting target "${identifier}.${target}"`,
{ identifier, target, element, attribute }
)
}
}
}
}
8 changes: 8 additions & 0 deletions packages/@stimulus/core/src/target_properties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@ export function TargetPropertiesBlessing<T>(constructor: Constructor<T>) {
}, {} as PropertyDescriptorMap)
}

export type TargetDescriptor = {
identifier: string | null
target: string | null
attribute: string
legacy: Boolean
element: Element
}

function propertiesForTargetDefinition(name: string) {
return {
[`${name}Target`]: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export class ApplicationTestCase extends DOMTestCase {
try {
this.setupApplication()
this.application.start()
this.application.warnings = false
await super.runTest(testName)
} finally {
this.application.stop()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ export default class ValueListObserverTests extends ObserverTestCase implements
this.recordCall("elementMatchedValue", element, value.id, value.token.content)
}

elementMatchedNoValue(token: Token) {
this.recordCall("elementMatchedNoValue", token)
}

elementUnmatchedValue(element: Element, value: Value) {
this.recordCall("elementUnmatchedValue", element, value.id, value.token.content)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export interface ValueListObserverDelegate<T> {
parseValueForToken(token: Token): T | undefined
elementMatchedValue(element: Element, value: T): void
elementUnmatchedValue(element: Element, value: T): void
elementMatchedNoValue(token: Token): void
}

interface ParseResult<T> {
Expand Down Expand Up @@ -54,6 +55,8 @@ export class ValueListObserver<T> implements TokenListObserverDelegate {
if (value) {
this.fetchValuesByTokenForElement(element).set(token, value)
this.delegate.elementMatchedValue(element, value)
} else {
this.delegate.elementMatchedNoValue(token)
}
}

Expand Down
1 change: 1 addition & 0 deletions packages/@stimulus/polyfills/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import "core-js/es/array/find"
import "core-js/es/array/find-index"
import "core-js/es/array/from"
import "core-js/es/array/includes"
import "core-js/es/map"
import "core-js/es/object/assign"
import "core-js/es/promise"
Expand Down