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

Took out most usages of process.env and replaced with config file. #161

Merged
merged 3 commits into from
Sep 15, 2020

Conversation

thai-truong
Copy link
Collaborator

Except for src/ServiceWorker.js and ./cypress/plugins because I don't
think it's a super good idea to change anything in those.

Except for src/ServiceWorker.js and ./cypress/plugins because I don't
think it's a super good idea to change anything in those.
@thai-truong thai-truong added the dev experience Focus on enhancing developer experience label Sep 12, 2020
@thai-truong thai-truong requested a review from a team September 12, 2020 22:39
@thai-truong thai-truong self-assigned this Sep 12, 2020
@netlify
Copy link

netlify bot commented Sep 12, 2020

Deploy preview for hknucsd-portal-dev ready!

Built with commit da132b2

https://deploy-preview-161--hknucsd-portal-dev.netlify.app

@@ -0,0 +1,23 @@
type Config = {
apiKey?: string;
authDomain?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i dont have it in my local .env and it's still fine

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

idk just part of the firebase config

src/config.ts Outdated
storageBucket?: string;
messagingSenderID?: string;
appID?: string;
apiBaseURL?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

apiURL should be good

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I should just remove this completely in some other pr, as discussed in some other comment below?

Copy link
Collaborator

Choose a reason for hiding this comment

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

we still need it for the ApiConfig store

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh wait nvm yeah

src/index.tsx Outdated
authDomain: config.authDomain,
databaseURL: config.databaseURL,
projectId: config.projectID,
storageBuck: config.storageBucket,
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happened to my storageBucket :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

someone ate the et D:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lmao :))


const SIGN_UP_URL = `${process.env.REACT_APP_API_BASE_URL}${SIGNUP_ENDPOINT}`;
const SIGN_UP_URL = `${config.apiBaseURL}${SIGNUP_ENDPOINT}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this even used? we should change this to the autogen stuff (can be for later pr)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah since we have autogen this is not needed anymore, including the function that actually uses this url. Will remove in some other pr

tsconfig.json Outdated
"src"
],
"extends": "./tsconfig.paths.json"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

THERE'S NO NEW LINE

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dude idek anymore newline kinda jank doe

@thai-truong thai-truong merged commit abf2501 into dev Sep 15, 2020
@thai-truong thai-truong linked an issue Sep 19, 2020 that may be closed by this pull request
godwinpang pushed a commit that referenced this pull request Sep 20, 2020
)

* Took out most usages of process.env and replaced with config file.

Except for src/ServiceWorker.js and ./cypress/plugins because I don't
think it's a super good idea to change anything in those.

* Made changes based on review+discussions from PR #161.
@godwinpang godwinpang deleted the env_config_file branch September 20, 2020 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev experience Focus on enhancing developer experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move process.env into config
2 participants