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

Pre-install node modules for Docker workflow #300

Merged
merged 13 commits into from
Apr 18, 2024

Conversation

GeorgeCodes19
Copy link
Contributor

@GeorgeCodes19 GeorgeCodes19 commented Feb 7, 2024

Ticket

No ticket

Changes

  • The Docker development workflow will now utilize a temporary Docker container to install dependencies on the host machine. This means that devs do not need to install node on their host machine and makes getting started with Docker a one-step command make dev.
  • Added path alias support to make the experience around relative paths more manageable

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

…r so that VS Code can properly resolve deps and provide intellisense. Added TS alias to resolve modules without using relative paths. Add default empty .env.local'
Copy link

github-actions bot commented Feb 7, 2024

Coverage report for app

St.
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 000bfd5

… explicitly declare NODE_ENV as 'development' so that dev dependencies and path alias resolution works within the Docker container
@GeorgeCodes19 GeorgeCodes19 marked this pull request as ready for review February 12, 2024 19:28
app/.env.development Outdated Show resolved Hide resolved
app/.env.development Outdated Show resolved Hide resolved
app/Makefile Outdated Show resolved Hide resolved
app/tsconfig.json Outdated Show resolved Hide resolved
app/Makefile Outdated Show resolved Hide resolved
@sawyerh sawyerh merged commit 803067e into main Apr 18, 2024
7 checks passed
@sawyerh sawyerh deleted the georgebyers/docker-requirements branch April 18, 2024 01:44
@rocketnova
Copy link
Contributor

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
Copy link
Contributor

sawyerh commented Apr 18, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor

sawyerh commented Apr 18, 2024

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

@rocketnova
Copy link
Contributor

@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?

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.

3 participants