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

Use SSR for every pages #2334

Merged
merged 7 commits into from Apr 7, 2023
Merged

Use SSR for every pages #2334

merged 7 commits into from Apr 7, 2023

Conversation

notmd
Copy link
Collaborator

@notmd notmd commented Apr 5, 2023

It's ready now. _document only runs once serverside, so this is the expected behavior we want. getInitialProps run on both server and client. On the client it populates the env wrong, it uses static env from .env instead of actual env, I'm not sure why.

Using SSR is the only way to make it work. This PR is quite similar to #2343 but I expose it to a global var for flexibility (accessing the env outside of react scope) and more performance.

@notmd notmd requested a review from AbdBarho as a code owner April 5, 2023 10:55
@AbdBarho AbdBarho marked this pull request as draft April 5, 2023 11:06
@notmd notmd changed the title Use ssr for all pages Inject inv vars via _document instead _app Apr 5, 2023
@notmd notmd changed the title Inject inv vars via _document instead _app Inject env vars via _document instead _app Apr 5, 2023
@notmd notmd marked this pull request as ready for review April 5, 2023 13:39
@notmd notmd marked this pull request as draft April 5, 2023 13:45
@AbdBarho
Copy link
Collaborator

AbdBarho commented Apr 5, 2023

On the client it populates the env wrong, it uses static env from .env instead of actual env, I'm not sure why.

Because the props are created at build time, where the ENABLE_CHAT variable is not available. If we change getStaticProps to getServerSideProps on /dashboard then the chat icon is visible because it forces a render in runtime, where the variable is available.

@notmd notmd changed the title Inject env vars via _document instead _app Use SSR for every pages Apr 6, 2023
@notmd notmd marked this pull request as ready for review April 6, 2023 16:21
@@ -15,5 +15,6 @@ export const config = {
"/leaderboard",
"/messages/:path*",
"/chat",
"/chat/:path*",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Required auth when access "/chat/[id]/

@@ -47,7 +47,7 @@ const styles: Styles = {
},
body: {
position: "relative",
bg: props.colorMode === "light" ? "gray.50" : colors.dark.bg,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fix a small UI issue where the color the not match on light mode.

Copy link
Collaborator

@AbdBarho AbdBarho left a comment

Choose a reason for hiding this comment

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

I am disappointed that next js does not allow for an easier solution for handling this.

maybe they expect everyone to have /api/config or similar anyway.

arguably speaking, that would still be the easiest solution.

@@ -24,3 +24,4 @@ ENABLE_EMAIL_SIGNIN=true
# inference config
INFERENCE_SERVER_HOST="http://localhost:8000"
INFERENCE_SERVER_API_KEY="6969"
ENABLE_CHAT=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

it should be disabled by default, otherwise the flag should be DISABLE_CHAT

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This env only used in local or static page (which we don't use any more). So I think it's fine

Copy link
Collaborator

@AbdBarho AbdBarho Apr 7, 2023

Choose a reason for hiding this comment

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

its fine by me, but, whoever is deploying the project should know that if they just delete the variable, chat will remain enabled, they have to explicitly set the value to false for it to be disabled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think those behaviors should be documented instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, please don't forget to document it somewhere!

website/src/pages/api/chat/assistant_message.ts Outdated Show resolved Hide resolved
@@ -9,6 +9,7 @@ declare global {
ADMIN_USERS: string;
MODERATOR_USERS: string;
INFERENCE_SERVER_HOST: string;
ENABLE_CHAT: boolean;
Copy link
Collaborator

@AbdBarho AbdBarho Apr 6, 2023

Choose a reason for hiding this comment

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

this should be string, after all, all are environment variables are strings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I make it boolean to ease the typing stuff. We only compare if(process.env.ENABLE_CHAT), and don't perform any string manipulation so having it as a string doesn't help anything here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then we need to remove all the boolean(...) from our code because it is redundant?

if (!boolean(process.env.ENABLE_CHAT)) {

But that would not work, right?

@notmd notmd enabled auto-merge (squash) April 7, 2023 15:21
@notmd notmd merged commit 491765f into main Apr 7, 2023
4 checks passed
@notmd notmd deleted the ssr_everything branch April 7, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants