search

EthanThatOneKid / acmcsuf.com

🐘 Official website of CSUF's ACM chapter
https://acmcsuf.com
MIT License
36 stars 45 forks source link

`npm run all` checks imply failure (status code 1) for non-env-holding users #1020

Open amyipdev opened 4 months ago

amyipdev commented 4 months ago

What happened?

When running npm run all, svelte-check yields 5 errors due to not having specific environment-variable secrets. While this is reasonable when running on-server, for development, this is unnecessary, and may confuse newer developers.

Recommendation: "soft fail" on environment variable-related svelte-check arguments.

amyipdev commented 4 months ago

Note: I am aware that the contributor's guide recommends users set up their .env, however my point still stands that these should be warnings, not hard failures. GCAL_API_KEY also fails and is not mentioned in the contributor guide.

EthanThatOneKid commented 4 months ago

and may confuse newer developers

I agree with the sentiment of this issue. Thanks for reporting 👍

EthanThatOneKid commented 3 months ago

While this is a limitation of SvelteKit's environment variable handling, there's a workaround. SvelteKit restricts environment variables to client-side, server-side, or both for security reasons.

As a temporary solution, you can copy all necessary variables from .env.example to .env. Just use empty strings ("") as placeholders for the missing values.

For more details on SvelteKit's environment variables, refer to the documentation: https://learn.svelte.dev/tutorial/env-dynamic-private.

amyipdev commented 3 months ago

What about adding a note to the README about that?

EthanThatOneKid commented 3 months ago

What about adding a note to the README about that?

The current instructions for setting up the .env file are in our CONTRIBUTING.md document. However, it's been a while since we've updated that section. Do you think it might need to be clearer or more detailed?

amyipdev commented 3 months ago

Definitely, it needs to be updated - it does not meet the current requirements.