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

refactor: pre-pano chores #539

Merged
merged 28 commits into from
Jul 26, 2023
Merged

refactor: pre-pano chores #539

merged 28 commits into from
Jul 26, 2023

Conversation

usirin
Copy link
Member

@usirin usirin commented Jul 22, 2023

Things that needs to be done before we can move onto #510

@usirin usirin requested a review from a team as a code owner July 22, 2023 21:54
@usirin usirin requested review from Luuwa and igdiaysu July 22, 2023 21:54
@vercel
Copy link

vercel bot commented Jul 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kampus-gql ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 26, 2023 5:25am
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
kampus-next ⬜️ Ignored (Inspect) Jul 26, 2023 5:25am
kampus-pasaport ⬜️ Ignored (Inspect) Jul 26, 2023 5:25am
kampus-ui ⬜️ Ignored (Inspect) Jul 26, 2023 5:25am

Copy link
Contributor

@Ketcap Ketcap left a comment

Choose a reason for hiding this comment

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

Great pr to start with and good documentation for all of us to see an example <3

apps/gql/clients/__mocks__/clients.ts Show resolved Hide resolved
apps/gql/loaders/user.ts Show resolved Hide resolved
@@ -0,0 +1,8 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

since we are using planet scale is this needed anymore ? i thought they are just using push on all changes so migrations are not needed. I might be wrong, don't know planet scale that much

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah this is mainly for local dev

@usirin
Copy link
Member Author

usirin commented Jul 26, 2023

You can follow the commits to figure out what's going on. I've tried my best to make sure each commit makes sense by their own.

Gonna merge this to move things forward.

cc @Ketcap @FurkanGM @cansirin

@usirin usirin merged commit 33f3e1a into dev Jul 26, 2023
2 checks passed
@usirin usirin deleted the pre-pano-chores branch July 26, 2023 05:37
Copy link
Contributor

@Ketcap Ketcap left a comment

Choose a reason for hiding this comment

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

Great work here, I have 2 comments only :)

@@ -1,13 +1,39 @@
import type { CodegenConfig } from "@graphql-codegen/cli";

const scalars = { Date: "string", DateTime: "string" };
Copy link
Contributor

Choose a reason for hiding this comment

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

Would "https://www.npmjs.com/package/graphql-scalars" help us with these ?

import { parse, stringify } from "@kampus/gql-utils/global-id";
import { type User } from "@kampus/prisma";
import { assertNever } from "@kampus/std";
import { type Dictionary } from "@kampus/std/dictionary";
Copy link
Contributor

Choose a reason for hiding this comment

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

should this type be exported from "@kampus/std" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

i thouht about it but since @kampus/std itself is gonna be lots of helpers/utils/modules (at least that's how i envision it), it's actually an umbrella package. so namespacing it to one level deeper, "/dictionary" for this example, should help us avoiding the namespace collisions. Think of it as importing from lodash/groupBy instead of lodash. Also helps the app to "tree shake" better (not even sure if it's the right term 😅 )

usirin added a commit that referenced this pull request Jul 30, 2023
I've changed the SozlukTerms query args in #539 but forgot to change
query we made in the sozluk query term page.

This also fixes the most stupidest npm bug i've ever encountered, i
won't even try to explain it but just know that it's so stupid and i
don't even know how we as an industry survive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants