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

Remove and ignore the node_modules directory #42

Closed
2zqa opened this issue Sep 21, 2023 · 3 comments
Closed

Remove and ignore the node_modules directory #42

2zqa opened this issue Sep 21, 2023 · 3 comments

Comments

@2zqa
Copy link

2zqa commented Sep 21, 2023

I noticed this repository has included the node_modules directory. This causes the git repository to be quite large in size. Are there any reasons for keeping it? I noticed the .gitignore file even explicitly allows it.

@kewisch
Copy link
Owner

kewisch commented Sep 24, 2023

I don't know if the recommendations have changed, but back in the early days of github actions, most docs said that node_modules needs to be added. Example still: https://docs.github.com/en/actions/creating-actions/creating-a-javascript-action#commit-tag-and-push-your-action-to-github . Granted that also says it can be problematic (without a why?), but I also don't want to start packing up javascript files.

Do you have any links on more current recommendations? Would love to better understand how things have changed.

@2zqa
Copy link
Author

2zqa commented Sep 25, 2023

Interesting to learn about that! I recently got into the development of GitHub actions and it seemed counter-intuitive to me. Primarily because the official typescript action template doesn't use it (https://github.com/actions/typescript-action), even though their simpler hello-world template does use it (https://github.com/actions/hello-world-javascript-action)

My other reasoning when I created this PR:

I see now it is not as clear-cut as I initially thought. I'll leave it up to you on whether to include this PR or not.

Footnotes

  1. https://github.com/actions/cache/blob/main/examples.md#node---npm

  2. https://github.com/actions/setup-node#caching-global-packages-data

@kewisch
Copy link
Owner

kewisch commented Sep 8, 2024

I'm closing this one as it still seems to be the most current guidance to include node_modules. There is an alternative that uses @vercel/ncc to compile the result into one package, but that doesn't work with web-ext since it uses a dynamic import for config modules, which trips up ncc. I commented on one of the open issues there, I'd pick that up in case there is a viable solution.

@kewisch kewisch closed this as completed Sep 8, 2024
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 a pull request may close this issue.

2 participants