navapbc / template-application-nextjs

Next.js, TypeScript, USWDS, and Storybook template, including CI/CD, for teams at Nava building web applications
Apache License 2.0
10 stars 3 forks source link

Pre-install node modules for Docker workflow #300

Closed GeorgeCodes19 closed 5 months ago

GeorgeCodes19 commented 7 months ago

Ticket

No ticket

Changes

Context for reviewers

Missing dependencies on the host machine

When opting to use Docker to develop and run the application the dependencies are only installed inside the container when building the image. This will cause IDEs to trigger an error about missing dependencies (node_modules) since they are not installed on the host machine.

Screenshot 2024-02-07 at 5 33 52 PM

Testing

Alias

Screenshot 2024-02-12 at 2 27 40 PM

resolution

Docker

The make dev command has been modified to check for the node_modules directory and remove any existing containers to avoid a container naming collision.

Screenshot 2024-02-12 at 2 25 27 PM
github-actions[bot] commented 7 months ago

Coverage report for app

St.:grey_question:
Category Percentage Covered / Total
🟡 Statements 76.54% 62/81
🟡 Branches 64.29% 9/14
🟡 Functions 71.43% 10/14
🟡 Lines 75% 54/72

Test suite run success

7 tests passing in 3 suites.

Report generated by 🧪jest coverage report action from 000bfd53bbf352c675676f7a46de9471bd342e66

rocketnova commented 5 months ago

I don't know if this is an issue for npm/node packages, but I've generally avoided doing this for package dependencies because packages sometimes aren't cross-platform compatible. Package managers typically do a good job of identifying which arch they need to be installing for. In this case, you're specifically installing packages that are good on debian, which may cause issues on Apple Silicon, etc

sawyerh commented 5 months ago

@rocketnova Yea good point. I think if the intent is for dependency files to be present so IDEs can find the imports, this might be fine

rocketnova commented 5 months ago

@sawyerh Do you think it's safe to assume that all developers working natively will be using IDEs? I don't 😅 but I also don't develop natively.

sawyerh commented 5 months ago

@rocketnova What do you use? I may not be fully understanding

rocketnova commented 5 months ago

@sawyerh Ah I see. I have misunderstood this issue. I apologize. I see that this PR is specifically for solving the local docker-based development workflow and making sure node_modules isn't empty because we're only bind mounting a subset of the development dir.

I think this PR approach is fine, but I would nit that we might want to rename the make target something more specific than make init. Perhaps make init-container?