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

Update .gitignore #20

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

KulkarniSuraj
Copy link
Contributor

@KulkarniSuraj KulkarniSuraj commented Oct 6, 2020

Adds more entries to .gitignore
https://github.com/github/gitignore/blob/master/Node.gitignore

since node_modules was created before adding .gitignore it is still being tracked
so node modules was deleted and then added to .gitignore

  • bug fixes

  • Enhancements

you can delete node_modules manually from your master branch

@prafulla-codes
Copy link
Owner

I think it is mandatory to update, the node_modules in case of a GitHub action, I read this somewhere, can you please confirm it @KulkarniSuraj

@KulkarniSuraj
Copy link
Contributor Author

I have read that npm ci does fresh installation everytime the action is run
and it is faster than npm install

@KulkarniSuraj
Copy link
Contributor Author

right now the git is tracking entire node modules and its subdirectories for a single commit
and it also increases the size of project

@rahil1304
Copy link
Contributor

rahil1304 commented Oct 6, 2020

Gitignore already had node_modules right?
So what's the point of adding the files individually to gitignore? Does adding files individually compared to the entire folder help in any way?

@KulkarniSuraj
Copy link
Contributor Author

I have read that npm ci does fresh installation everytime the action is run
and it is faster than npm install

npm ci is present in .github/workflows/node.js.yml
it removes node_modules if already present
refer to https://docs.npmjs.com/cli/ci.html

@KulkarniSuraj
Copy link
Contributor Author

Gitignore already had node_modules right?
So what's the point of adding the files individually to gitignore? Does adding files individually compared to the entire folder help in any way?

node_modules were still being tracked

@rahil1304
Copy link
Contributor

Gitignore already had node_modules right?
So what's the point of adding the files individually to gitignore? Does adding files individually compared to the entire folder help in any way?

node_modules were still being tracked

How was it being tracked? The .gitignore file had "node_modules" in it. So that means any changes to the node_modules folder was not being tracked by git.

@KulkarniSuraj
Copy link
Contributor Author

KulkarniSuraj commented Oct 6, 2020

Gitignore already had node_modules right?
So what's the point of adding the files individually to gitignore? Does adding files individually compared to the entire folder help in any way?

node_modules were still being tracked

How was it being tracked? The .gitignore file had "node_modules" in it. So that means any changes to the node_modules folder was not being tracked by git.

I had same doubt. but node_modules was present even before .gitignore
and I noticed that git was tracking the changes in node modules, I don't know why

@KulkarniSuraj
Copy link
Contributor Author

Gitignore already had node_modules right?
So what's the point of adding the files individually to gitignore? Does adding files individually compared to the entire folder help in any way?

node_modules were still being tracked

How was it being tracked? The .gitignore file had "node_modules" in it. So that means any changes to the node_modules folder was not being tracked by git.

please refer to this commit ede0d78

@prafulla-codes
Copy link
Owner

Ok @KulkarniSuraj, you mean actions can run using
npm ci, that way there is no need to track node_modules?

It would be great then, because it will reduce the size of the project considerably

Copy link
Owner

@prafulla-codes prafulla-codes left a comment

Choose a reason for hiding this comment

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

Interesting, i have some queries before I accept this PR

Do we need to put npm ci script somewhere explicitly so that it runs fine in production?

@prafulla-codes prafulla-codes added the enhancement New feature or request label Oct 7, 2020
@prafulla-codes prafulla-codes added this to the Express AutoDocs v0.0.2 milestone Oct 7, 2020
@KulkarniSuraj
Copy link
Contributor Author

@Pika1998 As you mentioned earlier node_modules might be required for javascript actions
many people are facing same problems with node_modules.
until Github solves this problem we can use @vercel/ncc or some other utility like this
which will compile all the code into single file which can be used by user.
This approach is also mentioned on javascript action page
Although this issue needs some more research I would like to know your thoughts on this

@prafulla-codes
Copy link
Owner

Ok as of now let's stick with the node_modules, I will try to do some research on the approach you are talking about and get back to you 😃

@KulkarniSuraj
Copy link
Contributor Author

Ok as of now let's stick with the node_modules, I will try to do some research on the approach you are talking about and get back to you 😃

Yeah! sure we can work on this later

@KulkarniSuraj
Copy link
Contributor Author

I was also thinking of adding test/output to .gitignore
because files in this folder are generated every time we run npm run dev
I was going to add those commits to this pull request
should I create separate pull request for that?

@prafulla-codes
Copy link
Owner

@KulkarniSuraj that's a nice idea, please make it as a separate PR

@prafulla-codes prafulla-codes mentioned this pull request Oct 10, 2020
8 tasks
@prafulla-codes prafulla-codes removed this from the Express AutoDocs v0.0.2 milestone Oct 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants