-
Notifications
You must be signed in to change notification settings - Fork 9
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
Loomio api #71
Loomio api #71
Conversation
Make downloads async Clean up .env var checking Ignore build/ in .gitignore
Update README
Ooops! Just saw the conflict with package-lock.json. Thought I had fixed that. But it should be easy to clear by running install. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Added some comments/suggestions. Mainly wondering about the .gitignore changes. Regarding the package-lock conflict, it would be great if you can merge master into this branch, run a new install to resolve the issue and push to update this PR :)
api/.gitignore
Outdated
npm-debug.log | ||
|
||
# API env | ||
.env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we deleting this stuff from the API .gitignore and moving it to the root .gitignore? Why are we not ignoring node_modules and npm-debug.log? Even if it's declared in the project's root .gitignore I think it makes sense to be explicit here too to reduce coupling between the api code and root. Ideally the ui and api code should function as their own projects, even though we happen to keep them in the same repo for convenience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this was a combination of ignorance and preference.
I would normally have a single .gitignore at the root level, and put all the rules there, so you would have
api/.env
but not
ui/.env
To me it seems simpler to manage them all in one place.
However, in line with your desire to make the modules as independent as possible, I can set it back to how it was and correct the other change which was made previously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about renaming the ui/.env file to something else, like "env" ?
My thinking on this is that I really don't want to accidentally check in a file with sensitive information because then you need to muck with git to permanently remove it.
If ui/.env is not sensitive information and it's okay to check it in, then it should be okay to make it a regular file, too. The dotenv package can do this:
require('dotenv').config({ path: '/full/custom/path/to/your/env/vars' })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about renaming the ui/.env file to something else, like "env" ?
My thinking on this is that I really don't want to accidentally check in a file with sensitive information because then you need to muck with git to permanently remove it.
I agree that this feels weird if you're used to using env variables on the backend, but variables defined in .env on the frontend will be included in the build of the code and available to the public, so should contain no secrets. The backend .env can contain secrets, but the frontend .env should not. You can see the recommendation about checking .env on the frontend into source control in the create-react-app documentation in the UI README, and read more about it here.
Changes to .gitignore configuration Fix await linting error Remove graphql calls to loomio loading functions
Okay, I've master with loomio-api and fixed the package-lock.json issue, and made the other changes requested. |
@LiamK Thanks for the changes! I resolved the fixed conversations but left the one about the ui .env file open so you can see my response. I also discovered a couple of new things (commented out code in loomio.js and mutations types in typeDefs), but I fixed those already :) |
In the loomio.js file, I needed to add
/* eslint-disable no-await-in-loop */
Because, uh, there was an await in a loop.
Will need to go back and do this differently. We can add all the infos in a batch instead of one-by-one.
I left the two initLoomio* functions in typeDefs.js. It is probably more convenient to do that now via the npm run init-loomio script, which I added later.