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: Build failing #54

Merged
merged 3 commits into from
Aug 11, 2024
Merged

fix: Build failing #54

merged 3 commits into from
Aug 11, 2024

Conversation

JowiAoun
Copy link
Collaborator

@JowiAoun JowiAoun commented Aug 10, 2024

This PR is meant to fix all build issues on the main branch since the initial commit.
Some of the bug fixes have been:

  • Cut out some of the boilerplate's post code, not entirely as at least one route is required. Its non-existence in the database caused build errors. This route should be fully discarded once a needed route is created.
  • Fix userInformation object in the authentication file, which had a duplicate userId attribute
  • Properly detecting next while running the development server.

Note: I haven't messed around with bun here, but it was just not installed on the repo. This still uses pnpm, and was aimed to only fix the build issues.

@JowiAoun JowiAoun linked an issue Aug 10, 2024 that may be closed by this pull request
Copy link

netlify bot commented Aug 10, 2024

Deploy Preview for cuhacking-portal-test-deployment ready!

Name Link
🔨 Latest commit ffe586d
🔍 Latest deploy log https://app.netlify.com/sites/cuhacking-portal-test-deployment/deploys/66b79cb087fd060008046008
😎 Deploy Preview https://deploy-preview-54--cuhacking-portal-test-deployment.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@HasithDeAlwis HasithDeAlwis left a comment

Choose a reason for hiding this comment

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

Approved 🫡 , let me know your thoughts on my questions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

From what I can tell, if we want to do this hot fix correctly, we should remove the post file all together. Doing this leads to some other issues we have to fix.
I understand why we haven't deleted it since appRouter needs to have procedures in it, otherwise we get other build errors. But for now, can't we define appRouter with a dummy procedure that just prints "hello, world"? But I think it leads to less confusion later on. Thoughts @JowiAoun?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It won't cause many problems to have it like this for a day or two. They're basically all dummy procedures now. I have a team route in PR #12 which I will re-build and request reviews for one last time, so we can finally deprecate this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perfect, that sounds good to me 👍

phone_number: null,
levels_of_study: null,
school: null,
userId: user.id,
}

await db.userInformation.create({
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know we are in progress to migrate to Drizzle, so do we care that this isn't creating a link between the UserInformation Table and the User table?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixing this is kind of annoying, we have to deconstruct all values of userInformation, except for user.id. We could do const { userId, ...otherUserInformation } = userInformation

data: {
      ...otherUserInformation,
      user: {
        connect: {
          id: userId,
        },
      },
    },

We'd obviously following better naming conventions. This is just an example of how we'd fix this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Weird enough, the schema has the id automatically defined, but is still required to be defined in the userInformation object. Since we are in the process of moving to Drizzle, I will leave it as is but good eye.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good to know, was just checking 👍

@HasithDeAlwis
Copy link
Collaborator

Im guessing these tests should be failing at the moment because of our issues with bun?

@JowiAoun
Copy link
Collaborator Author

JowiAoun commented Aug 10, 2024

Im guessing these tests should be failing at the moment because of our issues with bun?

After further testing with bun, it turns out it is still broken and therefore we should stay with pnpm until that is fixed.
Therefore Build / release (20) (pull_request) and End-to-End Tests / test are failing because they are still running with bun, but will function properly with pnpm. The priority right now is making sure we can make a proper build.

@HasithDeAlwis note: NEXTAUTH_SECRET is missing in the repo, which breaks Build / release (20) (pull_request)

@HasithDeAlwis
Copy link
Collaborator

Im guessing these tests should be failing at the moment because of our issues with bun?

After further testing with bun, it turns out it is still broken and therefore we should stay with pnpm until that is fixed. Therefore Build / release (20) (pull_request) and End-to-End Tests / test are failing because they are still running with bun, but will function properly with pnpm. The priority right now is making sure we can make a proper build.

@HasithDeAlwis note: NEXTAUTH_SECRET is missing in the deployment, which breaks Build / release (20) (pull_request)

Isn't this jsut because we don't have NEXAUTH_SECRET when we define the env?

Copy link
Member

@MFarabi619 MFarabi619 left a comment

Choose a reason for hiding this comment

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

Reviewed from my Android

@MFarabi619 MFarabi619 merged commit 9531107 into main Aug 11, 2024
4 of 7 checks passed
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.

[BUG] Build failing
3 participants