apply suggestions from code review

vaadin · Aug 6, 2024 · 0d2291e · 0d2291e
1 parent 7f01d49
commit 0d2291e
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 65 deletions.
diff --git a/packages/ts/generator-plugin-signals/src/SignalProcessor.ts b/packages/ts/generator-plugin-signals/src/SignalProcessor.ts
@@ -5,11 +5,6 @@ import DependencyManager from '@vaadin/hilla-generator-utils/dependencies/Depend
 import PathManager from '@vaadin/hilla-generator-utils/dependencies/PathManager.js';
 import ts, { type FunctionDeclaration, type SourceFile } from 'typescript';
 
-export type MethodInfo = Readonly<{
-  name: string;
-  signalType: string;
-}>;
-
 const HILLA_REACT_SIGNALS = '@vaadin/hilla-react-signals';
 
 const NUMBER_SIGNAL_CHANNEL = '$NUMBER_SIGNAL_CHANNEL$';
@@ -21,10 +16,10 @@ export default class SignalProcessor {
   readonly #dependencyManager: DependencyManager;
   readonly #owner: Plugin;
   readonly #service: string;
-  readonly #methods: MethodInfo[];
+  readonly #methods: Map<string, string>;
   readonly #sourceFile: SourceFile;
 
-  constructor(service: string, methods: MethodInfo[], sourceFile: SourceFile, owner: Plugin) {
+  constructor(service: string, methods: Map<string, string>, sourceFile: SourceFile, owner: Plugin) {
     this.#service = service;
     this.#methods = methods;
     this.#sourceFile = sourceFile;
@@ -41,54 +36,68 @@ export default class SignalProcessor {
     const [_p, _isType, connectClientId] = imports.default.find((p) => p.includes('connect-client'))!;
 
     this.#processSignalImports(signalImportPaths);
+    const initTypeId = imports.named.getIdentifier('@vaadin/hilla-frontend', 'EndpointRequestInit');
+    let initTypeUsageCount = 0;
 
     const [file] = ts.transform<SourceFile>(this.#sourceFile, [
-      ...this.#methods.map((method) =>
-        transform<SourceFile>((tsNode) => {
-          if (ts.isFunctionDeclaration(tsNode) && tsNode.name?.text === method.name) {
-            const body = template(
-              `
+      transform((tsNode) => {
+        if (ts.isFunctionDeclaration(tsNode) && tsNode.name && this.#methods.has(tsNode.name.text)) {
+          const methodName = tsNode.name.text;
+
+          const body = template(
+            `
 function dummy() {
-  return new ${NUMBER_SIGNAL_CHANNEL}('${this.#service}.${method.name}', ${CONNECT_CLIENT}).signal;
+  return new ${NUMBER_SIGNAL_CHANNEL}('${this.#service}.${methodName}', ${CONNECT_CLIENT}).signal;
 }`,
-              (statements) => (statements[0] as FunctionDeclaration).body?.statements,
-              [
-                transform((node) =>
-                  ts.isIdentifier(node) && node.text === NUMBER_SIGNAL_CHANNEL ? numberSignalChannelId : node,
-                ),
-                transform((node) => (ts.isIdentifier(node) && node.text === CONNECT_CLIENT ? connectClientId : node)),
-              ],
-            );
-
-            let returnType = tsNode.type;
-            if (
-              returnType &&
-              ts.isTypeReferenceNode(returnType) &&
-              'text' in returnType.typeName &&
-              returnType.typeName.text === 'Promise'
-            ) {
-              if (returnType.typeArguments && returnType.typeArguments.length > 0) {
-                returnType = returnType.typeArguments[0];
-              }
+            (statements) => (statements[0] as FunctionDeclaration).body?.statements,
+            [
+              transform((node) =>
+                ts.isIdentifier(node) && node.text === NUMBER_SIGNAL_CHANNEL ? numberSignalChannelId : node,
+              ),
+              transform((node) => (ts.isIdentifier(node) && node.text === CONNECT_CLIENT ? connectClientId : node)),
+            ],
+          );
+
+          let returnType = tsNode.type;
+          if (
+            returnType &&
+            ts.isTypeReferenceNode(returnType) &&
+            'text' in returnType.typeName &&
+            returnType.typeName.text === 'Promise'
+          ) {
+            if (returnType.typeArguments && returnType.typeArguments.length > 0) {
+              returnType = returnType.typeArguments[0];
             }
-
-            return ts.factory.createFunctionDeclaration(
-              tsNode.modifiers?.filter((modifier) => modifier.kind !== ts.SyntaxKind.AsyncKeyword),
-              tsNode.asteriskToken,
-              tsNode.name,
-              tsNode.typeParameters,
-              tsNode.parameters.filter(({ name }) => !(ts.isIdentifier(name) && name.text === 'init')),
-              returnType,
-              ts.factory.createBlock(body ?? [], false),
-            );
           }
 
-          return tsNode;
-        }),
-      ),
+          return ts.factory.createFunctionDeclaration(
+            tsNode.modifiers?.filter((modifier) => modifier.kind !== ts.SyntaxKind.AsyncKeyword),
+            tsNode.asteriskToken,
+            tsNode.name,
+            tsNode.typeParameters,
+            tsNode.parameters.filter(({ name }) => !(ts.isIdentifier(name) && name.text === 'init')),
+            returnType,
+            ts.factory.createBlock(body ?? [], false),
+          );
+        }
+        return tsNode;
+      }),
+      transform((tsNode) => {
+        if (ts.isFunctionDeclaration(tsNode)) {
+          if (
+            !(tsNode.name && this.#methods.has(tsNode.name.text)) &&
+            tsNode.parameters.some((p) => p.type && ts.isTypeReferenceNode(p.type) && p.type.typeName === initTypeId)
+          ) {
+            initTypeUsageCount += 1;
+          }
+        }
+        return tsNode;
+      }),
     ]).transformed;
 
-    this.#removeUnusedRequestInitImports(file);
+    if (initTypeUsageCount === 0) {
+      imports.named.remove('@vaadin/hilla-frontend', 'EndpointRequestInit');
+    }
 
     return createSourceFile(
       [
@@ -112,15 +121,4 @@ function dummy() {
       }
     });
   }
-
-  #removeUnusedRequestInitImports(file: SourceFile) {
-    const transformedFileText = ts.createPrinter().printFile(file);
-
-    const hasNormalEndpointCalls =
-      transformedFileText.includes('init?: EndpointRequestInit') && transformedFileText.includes('.call(');
-
-    if (!hasNormalEndpointCalls) {
-      this.#dependencyManager.imports.named.remove('@vaadin/hilla-frontend', 'EndpointRequestInit');
-    }
-  }
 }
diff --git a/packages/ts/generator-plugin-signals/src/index.ts b/packages/ts/generator-plugin-signals/src/index.ts
@@ -1,7 +1,7 @@
 import Plugin from '@vaadin/hilla-generator-core/Plugin.js';
 import type SharedStorage from '@vaadin/hilla-generator-core/SharedStorage.js';
 import type { OpenAPIV3 } from 'openapi-types';
-import SignalProcessor, { type MethodInfo } from './SignalProcessor.js';
+import SignalProcessor from './SignalProcessor.js';
 
 export type PathSignalType = Readonly<{
   path: string;
@@ -33,17 +33,14 @@ function extractEndpointMethodsWithSignalsAsReturnType(storage: SharedStorage):
     .reduce<PathSignalType[]>((acc, current) => acc.concat(current), []);
 }
 
-function groupByService(signals: readonly PathSignalType[]): Map<string, MethodInfo[]> {
+function groupByService(signals: readonly PathSignalType[]): Map<string, Map<string, string>> {
   return signals.reduce((serviceMap, signal) => {
     const [_, service, method] = signal.path.split('/');
-    const serviceMethods = serviceMap.get(service) ?? [];
-    serviceMethods.push({
-      name: method,
-      signalType: signal.signalType,
-    });
+    const serviceMethods = serviceMap.get(service) ?? new Map<string, string>();
+    serviceMethods.set(method, signal.signalType);
     serviceMap.set(service, serviceMethods);
     return serviceMap;
-  }, new Map<string, MethodInfo[]>());
+  }, new Map<string, Map<string, string>>());
 }
 
 export default class SignalsPlugin extends Plugin {