search

foo-software / lighthouse-check-action

GitHub Action for running @GoogleChromeLabs Lighthouse audits with all the bells and whistles đź”” Multiple audits, Slack notifications, and more!
https://github.com/marketplace/actions/lighthouse-check
MIT License
471 stars 24 forks source link

Add node_modules to .gitignore #107

Closed mcalthrop closed 1 year ago

mcalthrop commented 1 year ago

Is your feature request related to a problem? Please describe.

Why store node_modules in the repository? This is an anti-pattern.

It also makes it almost impossible to see the difference between releases of this package.

For example: https://github.com/foo-software/lighthouse-check-action/compare/v9.0.0...v9.1.0

Screenshot 2022-12-07 at 14 37 45

Describe the solution you'd like

The node_modules folder should be added to .gitignore.

Describe alternatives you've considered

N/A

Additional context

N/A

adamhenson commented 1 year ago

Trust me, I hate this more than you, since it seems that you haven't had the experience of authoring a JavaScript GitHub Action.

I agree now as I did over two years ago. Welcome to GitHub Actions authoring.

I'm closing this because it's not a specific pattern of this repo, but necessary per the docs in authoring a JavaScript based GitHub Action. Also, this is a duplicate of https://github.com/foo-software/lighthouse-check-action/issues/32

Related Links

Direct Reference

Creating a JavaScript action - GitHub Docs

GitHub downloads each action run in a workflow during runtime and executes it as a complete package of code before you can use workflow commands like run to interact with the runner machine. This means you must include any package dependencies required to run the JavaScript code. You'll need to check in the toolkit core and github packages to your action's repository.

From your terminal, commit your action.yml, index.js, node_modules, package.json, package-lock.json, and README.md files. If you added a .gitignore file that lists node_modules, you'll need to remove that line to commit the node_modules directory.

Attempt to Use @vercel/ncc

I did try everything in my power to prevent the need to version node_modules, including the one alternative provided by the docs, to implement @vercel/ncc. I ran into several issues seeming like they were bugs from @vercel/ncc. This was years ago, so I don't recall the details, but it was specific to the Lighthouse dependency. Certain parts of the Lighthouse package weren't being exported when using @vercel/ncc if I remember correctly.

adamhenson commented 1 year ago

This is an anti-pattern.

God... is that you? Please stop adding comments like this on any repo.

mcalthrop commented 1 year ago

Trust me, I hate this more than you, since it seems that you haven't had the experience of authoring a JavaScript GitHub Action.

I agree now as I did over two years ago. Welcome to GitHub Actions authoring.

I'm closing this because it's not a specific pattern of this repo, but necessary per the docs in authoring a JavaScript based GitHub Action. Also, this is a duplicate of #32

Related Links

Direct Reference

Creating a JavaScript action - GitHub Docs

GitHub downloads each action run in a workflow during runtime and executes it as a complete package of code before you can use workflow commands like run to interact with the runner machine. This means you must include any package dependencies required to run the JavaScript code. You'll need to check in the toolkit core and github packages to your action's repository.

From your terminal, commit your action.yml, index.js, node_modules, package.json, package-lock.json, and README.md files. If you added a .gitignore file that lists node_modules, you'll need to remove that line to commit the node_modules directory.

Attempt to Use @vercel/ncc

I did try everything in my power to prevent the need to version node_modules, including the one alternative provided by the docs, to implement @vercel/ncc. I ran into several issues seeming like they were bugs from @vercel/ncc. This was years ago, so I don't recall the details, but it was specific to the Lighthouse dependency. Certain parts of the Lighthouse package weren't being exported when using @vercel/ncc if I remember correctly.

@adamhenson Thanks for taking the time to explain this. What a pain. Didn't see the duplication, apologies.

adamhenson commented 1 year ago

No problem... I get it. And yes, it is disturbing - I'm with you there. Seeing this made me wonder if they've changed that requirement at all since then, or offered a better solution, but it appears not unfortunately.

mcalthrop commented 1 year ago

No problem... I get it. And yes, it is disturbing - I'm with you there. Seeing this made me wonder if they've changed that requirement at all since then, or offered a better solution, but it appears not unfortunately.

Bummer. Thanks for the GH action though!