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.

Sign up for GitHub

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

Jump to bottom

fix imports #58

Merged
merged 4 commits into from
Nov 21, 2024
Merged

fix imports #58

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
91619bf
add dynamic import
wtfdivyansh Nov 21, 2024
b26c867
Merge branch 'main' of https://github.com/wtfdivyansh/plura
wtfdivyansh Nov 21, 2024
8706a1f
add typesafe api
wtfdivyansh Nov 21, 2024
90e17b9
chore: format code with Prettier
github-actions[bot] Nov 21, 2024
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
2 changes: 1 addition & 1 deletion apps/api/app/api/[[...route]]/health.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Hono } from "hono";
const app = new Hono();
app.get("/", async (c) => {
app.get("/", (c) => {
return c.json({
message: "i am alive",
status: 200,
Expand Down
3 changes: 2 additions & 1 deletion apps/api/app/api/[[...route]]/mail.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { Hono } from "hono";
import { sendBatchEmail, sendEmail } from "@repo/mail";
import { mailBatchSchema, mailSchema } from "@repo/types";
import { zValidator } from "@hono/zod-validator";

Expand All @@ -8,6 +7,7 @@ const app = new Hono();
app
.post("/send", zValidator("json", mailSchema), async (c) => {
const { email, subject } = c.req.valid("json");
const { sendEmail } = await import("@repo/mail");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider caching the dynamic import result

The dynamic import is executed on every request, which could introduce unnecessary latency. Additionally, the import failure isn't explicitly handled.

Consider moving the import outside the route handler and implementing proper error handling:

+let sendEmailFn: typeof import("@repo/mail")["sendEmail"] | null = null;
+
 app
   .post("/send", zValidator("json", mailSchema), async (c) => {
     const { email, subject } = c.req.valid("json");
-    const { sendEmail } = await import("@repo/mail");
+    try {
+      if (!sendEmailFn) {
+        const { sendEmail } = await import("@repo/mail");
+        sendEmailFn = sendEmail;
+      }
+      const { data, error } = await sendEmailFn(email, subject);
+    } catch (importError) {
+      return c.json(
+        {
+          message: "Email service initialization failed",
+        },
+        500
+      );
+    }

Committable suggestion skipped: line range outside the PR's diff.

const { data, error } = await sendEmail(email, subject);
if (error) {
return c.json(
Expand Down Expand Up @@ -35,6 +35,7 @@ app
app
.post("/send-batch", zValidator("json", mailBatchSchema), async (c) => {
const { emails, subject } = c.req.valid("json");
const { sendBatchEmail } = await import("@repo/mail");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Apply similar caching pattern and improve error messages

The dynamic import pattern is duplicated here and could benefit from the same caching approach. Additionally, the error messages could be more specific for batch operations.

Consider implementing similar caching and improving error messages:

+let sendBatchEmailFn: typeof import("@repo/mail")["sendBatchEmail"] | null = null;
+
 app
   .post("/send-batch", zValidator("json", mailBatchSchema), async (c) => {
     const { emails, subject } = c.req.valid("json");
-    const { sendBatchEmail } = await import("@repo/mail");
+    try {
+      if (!sendBatchEmailFn) {
+        const { sendBatchEmail } = await import("@repo/mail");
+        sendBatchEmailFn = sendBatchEmail;
+      }
+      const { data, error } = await sendBatchEmailFn(emails, subject);
+      if (error) {
+        return c.json(
+          {
-            message: "Email sent failed",
+            message: "Batch email operation failed",
+            error: error.message
+          },
+          400
+        );
+      }
+    } catch (importError) {
+      return c.json(
+        {
+          message: "Email service initialization failed",
+        },
+        500
+      );
+    }

Committable suggestion skipped: line range outside the PR's diff.

const { data, error } = await sendBatchEmail(emails, subject);
if (error) {
return c.json(
Expand Down
4 changes: 2 additions & 2 deletions apps/api/app/api/[[...route]]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { Hono } from "hono";
import { auth as Auth } from "@repo/auth";
import { cors } from "hono/cors";
import mail from "./mail";
import hello from "./test";
import test from "./test";
import session from "./session";
import auth from "./auth";
import status from "./status";
Expand Down Expand Up @@ -38,7 +38,7 @@ app.use(

app.route("/health", health);
app.route("/session", session);
app.route("/test", hello);
app.route("/hello", test);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Renaming test route to hello is intentional but implementation needs alignment

The route change from /test to /hello is actually intentional as evidenced by the test file (hello.test.ts) which consistently uses /hello endpoint for testing user operations. However, there's a mismatch between:

  • The route name (/hello)
  • The handler file name (test.ts)
  • The handler's functionality (user operations)

Suggested changes:

  • Rename test.ts to users.ts or hello.ts to better reflect its purpose
  • Update the import statement accordingly
  • Consider renaming the route to /users to match the actual functionality
🔗 Analysis chain

Verify the intent of renaming test route to hello

There's an inconsistency between:

  • The module name (test)
  • The route path (/hello)
  • The handler variable name (test)

This makes the code less maintainable. Please verify if this was intentional and consider aligning these names.

Let me gather more information about the route handler and its usage.

Let me check the content of the test handler file and the test file to understand the full context.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the contents and usage of the test module to understand its purpose

# Check the implementation of the test module
echo "Checking test module implementation:"
cat apps/api/app/api/test.ts || echo "File not found"

# Look for any references to the old route
echo -e "\nChecking for references to old route path:"
rg -l "/test" --type ts

Length of output: 373

Script:

#!/bin/bash
# Check the content of the route file to understand the context
echo "Checking route file content:"
cat "apps/api/app/api/[[...route]]/route.ts"

# Look for any test-related files that might contain the handler
echo -e "\nLooking for test-related files:"
fd -t f -e ts test

# Search for 'test' function/variable declarations
echo -e "\nSearching for test handler definition:"
ast-grep --pattern 'const test = $_'

Length of output: 1670

Script:

#!/bin/bash
# Check the test handler implementation
echo "Checking test handler implementation:"
cat "apps/api/app/api/[[...route]]/test.ts"

# Check the test file content
echo -e "\nChecking test file content:"
cat "apps/api/tests/hello.test.ts"

Length of output: 3285

app.route("/mail", mail);
app.route("/auth", auth);
app.route("/status", status);
Expand Down
Loading