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

Loomio api #71

Merged
merged 15 commits into from
Nov 28, 2018
Merged

Loomio api #71

merged 15 commits into from
Nov 28, 2018

Conversation

LiamK
Copy link
Collaborator

@LiamK LiamK commented Nov 26, 2018

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.

@LiamK LiamK requested a review from erikfrisk November 26, 2018 15:17
@LiamK
Copy link
Collaborator Author

LiamK commented Nov 26, 2018

Ooops! Just saw the conflict with package-lock.json. Thought I had fixed that. But it should be easy to clear by running install.

Copy link
Collaborator

@erikfrisk erikfrisk left a 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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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' })

Copy link
Collaborator

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
@LiamK
Copy link
Collaborator Author

LiamK commented Nov 28, 2018

Okay, I've master with loomio-api and fixed the package-lock.json issue, and made the other changes requested.

@erikfrisk
Copy link
Collaborator

@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 :)

@erikfrisk erikfrisk merged commit d745c74 into master Nov 28, 2018
@erikfrisk erikfrisk deleted the loomio-api branch November 28, 2018 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants