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

feat: move env vars from build-time to run-time #789

Merged

Conversation

ShubhamPalriwala
Copy link
Contributor

@ShubhamPalriwala ShubhamPalriwala commented Sep 8, 2023

What does this PR do?

Shifts a set ( SignUp and SignIn ) related environment variables from Build-time configuration to Run-time configuration! This will allow users to edit these, now, in their docker/runtiem env itself instead of needing to build the image from source.

This is possible because we now parse the env var in server side pages and pass them as a prop to the child components who are client side!

Warning: We need to update our env variables of the hosted app as well!

This PR migrates:

  • EMAIL_VERIFICATION_DISABLED
  • PASSWORD_RESET_DISABLED
  • SIGNUP_DISABLED
  • INVITE_DISABLED
  • PRIVACY_URL
  • TERMS_URL
  • IMPRINT_URL
  • GITHUB_AUTH_ENABLED
  • GOOGLE_AUTH_ENABLED
  • WEBAPP_URL
  • IS_FORMBRICKS_CLOUD
  • SURVEY_BASE_URL

Have updated the documentation as well!

Migration Guide

Screenshot_2023-09-11-08-52-21_5760x1080
Screenshot_2023-09-11-08-52-44_5760x1080
Screenshot_2023-09-11-08-53-08_5760x1080

Type of change

  • Enhancement (small improvements)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • Added a screen recording or screenshots to this PR
  • Filled out the "How to test" section in this PR
  • Read the contributing guide
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand bits
  • Ran pnpm build
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues
  • Updated the Formbricks Docs if changes were necessary

@vercel
Copy link

vercel bot commented Sep 8, 2023

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

Name Status Preview Comments Updated (UTC)
formbricks-cloud ❌ Failed (Inspect) Sep 30, 2023 6:31pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
formbricks-com ⬜️ Ignored (Inspect) Visit Preview Sep 30, 2023 6:31pm

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2023

Thank you for following the naming conventions for pull request titles! 🙏

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2023

📦 Next.js Bundle Analysis for @formbricks/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

Copy link
Member

@mattinannt mattinannt left a comment

Choose a reason for hiding this comment

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

@ShubhamPalriwala looks great 😊💪 I added a few comments, let's chat about it or if they make sense please implement the changes.
Also please pull the latest changes from main and solve the merge conflicts 😊

HEROKU_URL ||
RENDER_URL ||
"http://localhost:3000";
export const WEBAPP_URL = process.env.WEBAPP_URL || "http://localhost:3000";
Copy link
Member

Choose a reason for hiding this comment

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

I now remember that the VERCEL_URL makes sense here due to get Formbricks working in a Vercel preview deployment where URLs change with every deployment. Only with VERCEL_URL we are able to set this correctly.

export const WEBAPP_URL = process.env.WEBAPP_URL || VERCEL_URL || "http://localhost:3000";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oo nice, done!

@@ -9,14 +10,24 @@ export const metadata: Metadata = {
};

export default function SignInPage() {
const publicSignUpEnabled = env.SIGNUP_DISABLED !== "1";
Copy link
Member

Choose a reason for hiding this comment

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

Would it maybe make sense to move the definition of the variables, e.g.
const publicSignUpEnabled = env.SIGNUP_DISABLED !== "1";
to the constants.ts file so we don't have to make these comparisons all over again?
But then we need to make sure that they are not imported variables are on the server.

what would you think about a constants-server.ts file with the import of the server-only package to prevent importing them at the client and moving all the server side environment variables/constants there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a nice idea!

Also, the "use server" will not work bec it only allows exporting async functions and not const vals. Remember facing this issue when we were trying to export an interface from a service file!

Update, I took a different approach and instead of having a separate constants file, I imported all those env vars into server components and passed them as props to child client components! 😎

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh okay, my bad, done!

…into shubham/for-1254-try-moving-build-time-vars-to-runtime
@mattinannt mattinannt merged commit 13afba7 into main Sep 30, 2023
10 of 12 checks passed
@mattinannt mattinannt deleted the shubham/for-1254-try-moving-build-time-vars-to-runtime branch September 30, 2023 19:41
waseemrabani pushed a commit to LuminosoInsight/formbricks that referenced this pull request Feb 2, 2024
…f-hosting environments (formbricks#789)

* feat: privacy, imprint, and terms URL env vars now do not need rebuilding

* feat: disable_singup env var now do not need rebuilding

* feat: password_reset_disabled env var now do not need rebuilding

* feat: email_verification_disabled env var now do not need rebuilding

* feat: github_oauth & google_oauth env var now do not need rebuilding

* feat: move logic of env vars to serverside and send boolean client-side

* feat: invite_disabled env var now do not need rebuilding

* feat: rename vars logically

* feat: migration guide

* feat: update docker-compose as per v1.1

* deprecate: unused NEXT_PUBLIC_VERCEL_URL & VERCEL_URL

* deprecate: unused RAILWAY_STATIC_URL

* deprecate: unused RENDER_EXTERNAL_URL

* deprecate: unused HEROKU_APP_NAME

* fix: define WEBAPP_URL & replace NEXT_WEBAPP_URL with it

* migrate: NEXT_PUBLIC_IS_FORMBRICKS_CLOUD to IS_FORMBRICKS_CLOUD

* chore: move all env parsing to a constants.ts from page files

* feat: migrate client side envs to server side

* redo: isFormbricksCloud to navbar serverside page

* fix: constants is now a server only file

* fix: removal of use swr underway

* fix: move 1 tag away from swr to service

* feat: move away from tags swr

* feat: move away from surveys  swr

* feat: move away from eventClass swr

* feat: move away from event swr

* fix: make constants server-only

* remove comments from .env.example, use constants in MetaInformation

* clean up services

* rename tag function

* fix build error

* fix smaller bugs, fix Response % not working in summary

---------

Co-authored-by: Matthias Nannt <mail@matthiasnannt.com>
waseemrabani pushed a commit to LuminosoInsight/formbricks that referenced this pull request Feb 2, 2024
…f-hosting environments (formbricks#789)

* feat: privacy, imprint, and terms URL env vars now do not need rebuilding

* feat: disable_singup env var now do not need rebuilding

* feat: password_reset_disabled env var now do not need rebuilding

* feat: email_verification_disabled env var now do not need rebuilding

* feat: github_oauth & google_oauth env var now do not need rebuilding

* feat: move logic of env vars to serverside and send boolean client-side

* feat: invite_disabled env var now do not need rebuilding

* feat: rename vars logically

* feat: migration guide

* feat: update docker-compose as per v1.1

* deprecate: unused NEXT_PUBLIC_VERCEL_URL & VERCEL_URL

* deprecate: unused RAILWAY_STATIC_URL

* deprecate: unused RENDER_EXTERNAL_URL

* deprecate: unused HEROKU_APP_NAME

* fix: define WEBAPP_URL & replace NEXT_WEBAPP_URL with it

* migrate: NEXT_PUBLIC_IS_FORMBRICKS_CLOUD to IS_FORMBRICKS_CLOUD

* chore: move all env parsing to a constants.ts from page files

* feat: migrate client side envs to server side

* redo: isFormbricksCloud to navbar serverside page

* fix: constants is now a server only file

* fix: removal of use swr underway

* fix: move 1 tag away from swr to service

* feat: move away from tags swr

* feat: move away from surveys  swr

* feat: move away from eventClass swr

* feat: move away from event swr

* fix: make constants server-only

* remove comments from .env.example, use constants in MetaInformation

* clean up services

* rename tag function

* fix build error

* fix smaller bugs, fix Response % not working in summary

---------

Co-authored-by: Matthias Nannt <mail@matthiasnannt.com>
waseemrabani pushed a commit to LuminosoInsight/formbricks that referenced this pull request Feb 2, 2024
…f-hosting environments (formbricks#789)

* feat: privacy, imprint, and terms URL env vars now do not need rebuilding

* feat: disable_singup env var now do not need rebuilding

* feat: password_reset_disabled env var now do not need rebuilding

* feat: email_verification_disabled env var now do not need rebuilding

* feat: github_oauth & google_oauth env var now do not need rebuilding

* feat: move logic of env vars to serverside and send boolean client-side

* feat: invite_disabled env var now do not need rebuilding

* feat: rename vars logically

* feat: migration guide

* feat: update docker-compose as per v1.1

* deprecate: unused NEXT_PUBLIC_VERCEL_URL & VERCEL_URL

* deprecate: unused RAILWAY_STATIC_URL

* deprecate: unused RENDER_EXTERNAL_URL

* deprecate: unused HEROKU_APP_NAME

* fix: define WEBAPP_URL & replace NEXT_WEBAPP_URL with it

* migrate: NEXT_PUBLIC_IS_FORMBRICKS_CLOUD to IS_FORMBRICKS_CLOUD

* chore: move all env parsing to a constants.ts from page files

* feat: migrate client side envs to server side

* redo: isFormbricksCloud to navbar serverside page

* fix: constants is now a server only file

* fix: removal of use swr underway

* fix: move 1 tag away from swr to service

* feat: move away from tags swr

* feat: move away from surveys  swr

* feat: move away from eventClass swr

* feat: move away from event swr

* fix: make constants server-only

* remove comments from .env.example, use constants in MetaInformation

* clean up services

* rename tag function

* fix build error

* fix smaller bugs, fix Response % not working in summary

---------

Co-authored-by: Matthias Nannt <mail@matthiasnannt.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants