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

fix: Some recently discovered type errors breaking build #18432

Merged
merged 1 commit into from
Dec 31, 2024

Conversation

emrysal
Copy link
Contributor

@emrysal emrysal commented Dec 31, 2024

(Haven't got a clue how this went undetected)

Detected async from #18431; but expands on it with additional lint/type fixes.

@cnhhoang850 for visibility; incorporates your detection also 🙏

@emrysal emrysal requested a review from Praashh December 31, 2024 13:25
@graphite-app graphite-app bot requested a review from a team December 31, 2024 13:26
@dosubot dosubot bot added the 🐛 bug Something isn't working label Dec 31, 2024
@keithwillcode keithwillcode added core area: core, team members only foundation labels Dec 31, 2024
@@ -20,7 +20,7 @@ const inputSchema = z.object({
cardKeys: z.array(z.string()),
});

export async function handler(request: Request) {
async function handler(request: Request) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Routes aren't allowed to export other functions than GET/POST/PUT..

@@ -33,7 +33,7 @@ const createI18nInstance = async (locale: string, ns: string) => {
return _i18n;
};

const getTranslationWithCache = async (locale: string, ns: string = "common") => {
const getTranslationWithCache = async (locale: string, ns = "common") => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ns: string = "common" resulted in a lint fail due to string being inferred.

@@ -1,4 +1,4 @@
import { PageProps } from "app/_types";
import type { PageProps } from "app/_types";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing type keyword.

@@ -1,6 +1,4 @@
import withEmbedSsrAppDir from "app/WithEmbedSSR";
import type { PageProps as _PageProps } from "app/_types";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused imports (@hbjORbj possibly a local lint issue?)

Copy link
Contributor

@Praashh Praashh Dec 31, 2024

Choose a reason for hiding this comment

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

yes I think, it's a lint issue.

@@ -76,6 +76,7 @@ export default async function RootLayout({ children }: { children: React.ReactNo
<script
nonce={nonce}
id="headScript"
// eslint-disable-next-line react/no-danger
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was legit missing, but no idea how we missed this

@@ -1,3 +1,4 @@
import type { EmbedProps } from "app/WithEmbedSSR";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Order incorrect.

@@ -14,7 +14,7 @@ import { Icon, TableNew, TableBody, TableCell, TableHead, TableHeader, TableRow

import { usePersistentColumnResizing } from "../lib/resizing";

export interface DataTableProps<TData, TValue> {
export type DataTableProps<TData, TValue> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NIT

@@ -89,7 +89,8 @@ export function DataTable<TData, TValue>({
usePersistentColumnResizing({
enabled: Boolean(enableColumnResizing),
table,
identifier,
// TODO: Figure out why 'identifier' somehow types to string | null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eunjae-lee - this is a weird one; would love for you to have a looksy

Copy link
Contributor

Choose a reason for hiding this comment

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

on it !

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the typedef from next.js seems to be weird.

Screenshot.2025-01-02.at.11.48.10.mp4

fixing it here:
c766694

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So usePathname is incorrectly typed? Is this a Nextjs version issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

@emrysal
I didn't dig deep enough, but the latest version seems to be the same.

The doc says it can return null. As my video above shows, it has string here and also string | null there. Not sure how exactly the two different types emerge here.

Screenshot 2025-01-03 at 10 24 04

https://nextjs.org/docs/app/api-reference/functions/use-pathname

Copy link

graphite-app bot commented Dec 31, 2024

Graphite Automations

"Add foundation team as reviewer" took an action on this PR • (12/31/24)

1 reviewer was added to this PR based on Keith Williams's automation.

Copy link
Contributor

@Praashh Praashh left a comment

Choose a reason for hiding this comment

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

LGTM 🙏 !!

@emrysal emrysal merged commit 890fc92 into main Dec 31, 2024
66 of 90 checks passed
@emrysal emrysal deleted the fix/bunch-of-build-errors-in-main branch December 31, 2024 13:50
Copy link
Contributor

E2E results are ready!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working core area: core, team members only foundation ready-for-e2e
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants