refactored to warningHandler and use appropriatly

reverted change
hotwired · Jun 16, 2023 · e43fdc1 · e43fdc1
1 parent e710672
commit e43fdc1
Show file tree

Hide file tree

Showing 10 changed files with 57 additions and 47 deletions.
diff --git 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
@@ -93,7 +94,7 @@ export class Application implements ErrorHandler {
 
   // Logging
 
-  handleWarning(warning: string, message: string, detail: object) {
+  handleWarning(warning: string, message: string, detail: any) {
     if (this.warnings) {
       this.logger.warn(`%s\n\n%s\n\n%o`, message, warning, detail)
     }

diff --git a/src/core/context.ts b/src/core/context.ts
@@ -146,6 +146,6 @@ export class Context implements ErrorHandler, TargetObserverDelegate, OutletObse
   }
 
   handleElementMatchedNoValue(element: Element, token: Token, error?: Error) {
-    this.application.router.scopeObserver.elementMatchedNoValue(element, token, error)
+    this.application.router.handleWarningsWithDuplicates(element, token, error)
   }
 }
diff --git 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<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) {
     let warnedKeys: Set<string> | 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, object)
     }
   }
 }
diff --git 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
-  public scopeObserver: ScopeObserver
+  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() {
@@ -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,10 +86,25 @@ export class Router implements ScopeObserverDelegate {
     this.application.handleError(error, message, detail)
   }
 
+  handleWarningsWithDuplicates(element: Element, token: Token, error?: Error) {
+    if (!error) {
+      const parsed = Action.forToken(token, this.schema)
+
+      if (!this.modules.map((c) => c.identifier).includes(parsed.identifier)) {
+        this.guide.warn(
+          element,
+          `identifier:${parsed.identifier}`,
+          `Warning connecting "${token.content}" to undefined controller "${parsed.identifier}"`,
+          `Warning connecting "${token.content}" to undefined controller "${parsed.identifier}"`
+        )
+      }
+    }
+  }
+
   // 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) {

diff --git 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
@@ -2,8 +2,6 @@ import { ErrorHandler } from "./error_handler"
 import { Schema } from "./schema"
 import { Scope } from "./scope"
 import { Token, ValueListObserver, ValueListObserverDelegate } from "../mutation-observers"
-import { Action } from "./action"
-import { Router } from "./router"
 
 export interface ScopeObserverDelegate extends ErrorHandler {
   createScopeForElementAndIdentifier(element: Element, identifier: string): Scope
@@ -18,7 +16,6 @@ export class ScopeObserver implements ValueListObserverDelegate<Scope> {
   private valueListObserver: ValueListObserver<Scope>
   private scopesByIdentifierByElement: WeakMap<Element, Map<string, Scope>>
   private scopeReferenceCounts: WeakMap<Scope, number>
-  private warnedSet: WeakMap<Element, string[]>
 
   constructor(element: Element, schema: Schema, delegate: ScopeObserverDelegate) {
     this.element = element
@@ -27,7 +24,6 @@ export class ScopeObserver implements ValueListObserverDelegate<Scope> {
     this.valueListObserver = new ValueListObserver(this.element, this.controllerAttribute, this)
     this.scopesByIdentifierByElement = new WeakMap()
     this.scopeReferenceCounts = new WeakMap()
-    this.warnedSet = new WeakMap()
   }
 
   start() {
@@ -65,29 +61,6 @@ export class ScopeObserver implements ValueListObserverDelegate<Scope> {
     }
   }
 
-  elementMatchedNoValue(element: Element, token: Token, error?: Error) {
-    if (!error) {
-      const parsed = Action.forToken(token, this.schema)
-      const router = this.delegate as Router
-
-      if (!router.modules.map((c) => c.identifier).includes(parsed.identifier)) {
-        if (!this.warnedSet.has(element)) {
-          this.warnedSet.set(element, [])
-        }
-
-        if (!this.warnedSet.get(element)?.includes(parsed.identifier)) {
-          router.application.handleWarning(
-            `Warning connecting identifier: ${parsed.identifier} action ${token.content}`,
-            `Warning connecting action ${token.content} with identifier: ${parsed.identifier}`,
-            { element, token }
-          )
-
-          this.warnedSet.get(element)?.push(parsed.identifier)
-        }
-      }
-    }
-  }
-
   elementUnmatchedValue(element: Element, value: Scope) {
     const referenceCount = this.scopeReferenceCounts.get(value)
     if (referenceCount) {
@@ -106,4 +79,6 @@ export class ScopeObserver implements ValueListObserverDelegate<Scope> {
     }
     return scopesByIdentifier
   }
+
+  elementMatchedNoValue() {}
 }
diff --git 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}`,
+        `Deprecated 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
@@ -0,0 +1,3 @@
+export interface WarningHandler {
+  handleWarning(warning: string, message: string, detail: any): void
+}
diff --git 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
@@ -90,12 +90,21 @@ export default class WarningTests extends ApplicationTestCase {
     this.assert.equal(this.mockLogger.warns.length, 2)
 
     this.assert.deepEqual(
-      this.mockLogger.warns.map((warn) => ({ message: warn.message })),
+      this.mockLogger.warns
+        .map((warn) => ({ message: warn.message, warning: warn.warning }))
+        .sort((warn) => warn.warning),
       [
-        { message: "Warning connecting action non-existing-controller#found with identifier: non-existing-controller" },
         {
+          warning:
+            'Warning connecting "non-existing-controller#found" to undefined controller "non-existing-controller"',
           message:
-            "Warning connecting action non-existing-controller#not-found with identifier: non-existing-controller",
+            'Warning connecting "non-existing-controller#found" to undefined controller "non-existing-controller"',
+        },
+        {
+          warning:
+            'Warning connecting "non-existing-controller#not-found" to undefined controller "non-existing-controller"',
+          message:
+            'Warning connecting "non-existing-controller#not-found" to undefined controller "non-existing-controller"',
         },
       ]
     )