Use string compairson for the env var DISABLE_REGISTRATION so that it works properly #827
Conversation
…v var DISABLE_REGISTRATION so that it works properly
|
@dattas is attempting to deploy a commit to the Listinai Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes update the logic for checking the Changes
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/backend/src/services/auth/auth.service.tsOops! Something went wrong! :( ESLint: 8.57.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@eslint/eslintrc' imported from /eslint.config.mjs apps/frontend/src/app/(app)/auth/page.tsxOops! Something went wrong! :( ESLint: 8.57.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@eslint/eslintrc' imported from /eslint.config.mjs 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I don't see what is the problem with the current implementation. |
|
That is the problem, if it's "exists" then it's "true", even if it's set to "false", so this causes a lot of problems |
|
What happens on some computer where false is translated as "false". |
|
Typically when your application uses a lot of env vars, you'd want all environment variables to be parsed by a configuration class. Then you'd call to that class to ensure consistent processing of the variable instead of having to parse each one each time you want to check (and you can handle logic in that single place instead of having to repeat the logic in the multiple places). We can certainly create a class like this so you are not having to pull raw env vars anywhere you want to use them. I simply wrote something minimal that will fix an existing open issue and something that tripped me up due to the behavior being inconsistent with the application's documentation and with most software's established standards. If I used something like nodejs workers, the existing implementation where you want the env variable to exist or not would not work properly as you can use the nodejs workers to pass non-string environment variables. I believe it is better to have a consistent and established behavior and documentation. If someone runs Postiz and manages to pass a boolean to the application instead of a string (like using a nodejs worker), they're outside normal operations and I'd argue supported setup of the application. Since the docker is the recommended setup, it isn't possible (that I'm aware of) for a user to get a boolean to not be coerced into a string past docker/docker compose and past pm2 (it'd just become a string) so trying to worry about unsupported setups isn't something I'd personally spend a lot of time worrying about people encountering. With that said, it is currently very confusing not only in the documentation of Postiz ( https://docs.postiz.com/configuration/reference ) indicating an example is I implemented this function using javascript's strict equality operator, the === or !== to ensure that it will not translate truthy like a == may explicitly to avoid confusion like corrupted environments causing problems. If you want to handle edge cases like you're describing I can change it to use something like yn ( https://www.npmjs.com/package/yn ) which is quite established package to ensure that a variety of values is checked to allow the handling of things that can be coerced safely into a boolean. I just assumed that an extra dependency to make the behavior match the documentation and I'd argue most people's expectations was undesired. Let me know if you'd like me to update the PR with something like yn and I'd happily do so! |
What kind of change does this PR introduce?
Bug fix, #730
Why was this change needed?
I ran into an issue where DISABLE_REGISTRATION was set to false, but it still acted as though it was set to true
Other information:
I didn't talk to anybody, I just saw the issue, figured out why it happened, and wanted to fix it
Checklist:
Put a "X" in the boxes below to indicate you have followed the checklist;
Summary by CodeRabbit