starter1 #1
starter1 #1
Conversation
andy9775
requested review from
andy9775 and
gurvirsasan
and removed request for
andy9775 and
gurvirsasan
|
@danielduong8 Can you squash your commits and use a more descriptive commit message please. |
.dockerignore
Outdated
| @@ -0,0 +1,2 @@ | |||
| */node_modules* | |||
There was a problem hiding this comment.
Docker will use the .gitignore file so this isn't needed (once you move the .gitignore file into root)
There was a problem hiding this comment.
I removed the .dockerignore as well as the "npm run build" commands to build the container and when I checked the container fs, the /build and /node_modules folders were still there and the website was still accessible which I assume means it simply copied them from my local directory.
I'm not sure if it really makes a significant difference but I added the .dockerignore so Docker wouldn't copy my local build and node_modules folder which takes time, since Docker was going to do "npm run build" anyways.
plausibly
force-pushed
the
starter1
branch
2 times, most recently
from
6b36c3b
to
a323b15
Compare
|
Sorry about all the commented code and poor commit messages, everything seemed to be working locally and I wanted to test if it would work on Heroku so I had to push in order to test. I was considering after getting the foundations fully set up, I would set up another clean branch that would be production ready and able to be pushed and reviewed. Right now the current issue causing the website to not work is that Heroku randomly assigns a port to the container, so I have to be able to retrieve that from ENV and tell Nginx to listen to that port. It's not working because the resource I was looking at suggested to use <%= ENV["port"] %> but I now realize that was ruby syntax and that won't work, unless I change the extension of the file or find another way. This, and all the other issues you mentioned as well. |
0x90-enjoyer
force-pushed
the
starter1
branch
from
e590845
to
5d683da
Compare
0x90-enjoyer
force-pushed
the
starter1
branch
from
6d80ace
to
691de9b
Compare
Set up basic directory structure with client and server folders Use npm to install server dependencies like cors, dotenv, express, and pg. Create react app using typescript template. Add Procfile with bin/start-nginx and npm start commands. Add start scripts in /package.json which touch /tmp/app-initialized file. Add heroku-postbuild script to /package.json. Add /tmp/nginx.socket port and listen to it in server.js. As Heroku searches for a Procfile to run the app, the Procfile will use bin/start-nginx to intialize nginx, then it will use npm start to call the scripts from the package.json to update server modules and start it. Touching /tmp/app-initialized is required to let nginx webpack know to start listening. After Heroku builds the application the heroku-postbuild script will be called and will run the client build. Nginx proxies to the node server through the /tmp/nginx.socket.
Add Dockerfile, app.json, heroku.yml. Replace old nginx.conf.erb files with new nginx config files. Update package.json and server.js. Limitations became apparent deploying files directly without Docker as testing became tedious with constant pushes to test changes and the nginx webpack provided by Heroku was unnatural and difficult to work with, in addition to having limited documentation. Transitioning to Docker also opens the door to useful features on Heroku such as their pipeline and "Review Apps". Use multi-stage Dockerfile build with node as first stage and nginx as the second and final stage. Client and server are built together in first stage as they both use npm. Copy local nginx configs to the container to replace default configs. Copy result from stage 1 in stage 2 as well as package.json in order to later run the scripts to start node and nginx. Install bash and npm into the container OS as bash allows multiple commands to be run and npm is needed as the nginx os does not have node by default which we need to run the node server. Deployment from containers on Heroku requires app.json and heroku.yml files in order to instruct Heroku and the "Review App" function on how to deploy the container. Base new nginx configs on a template from nginx's official website and as they seem to be much more functional in terms of reverse proxying and serving static assets. Add postinstall script to package.json to be called by CMD command in Dockerfile. Change listening port in server.js as nginx should proxy to port 8080 from its port.
- Pass Heroku's randomly assigned port to config files at runtime As Heroku randomly assigns ports to its containers before deploying, nginx needs to assign the port at runtime and can't be statically defined. Thus, make a template config file locally, copy it into container, then during runtime, retrieve the port environment variable and the template to create appropriate default.conf file for nginx to use. - Simplify client and server building in Dockerfile. The build steps for frontend and backend can be isolated into their own logic. Seperate their build steps. Previously, only "npm install" would be called which led to a postinstall in package.json to call run build for both client and server which was convoluted. Instead, move "npm run build" commands to Dockerfile, and also use "--production" argument to ignore unnecessary devDependencies. - Add README.md file As Dockerfile now uses a variable port number, "docker build" needs to be passed "-e PORT=80" argument in order to be run locally. Therefore, add appropriate instructions to README.md. - Combine /.gitignore and /client/.gitignore As having both is redundant, delete /client.gitignore and move "build/" to /.gitignore. - Remove Procfile As the new method of deployment uses Docker, Procfile has become obsolete so delete it. - Add TypeScript support for server As typescript support was only installed in client, install TypeScript into server using "npm install typescript --save-dev" and "npx tsc --init. Convert server.js to server.ts, modify syntax, and add type annotations as appropriate. As TypeScript needs to be aware of types of imported modules, use "npm i --save-dev @types/_package_". - Remove commented code Files are filled with commented code and cookiecutter template comments, which clutter the codebase and commented code is bad practice when using git. Remove them.
- Add .dockerignore file .dockerignore was removed in previous commit due to assumption that Docker already reads from .gitignore. After testing, this does not appear to be the case. Re-add the .dockerignore file to prevent unnecessary copying of /build and /node_modules. - Change build target in heroku.yml heroku.yml used the old build target "builder". Update the build target to "client_builder" to match Dockerfile.
0x90-enjoyer
force-pushed
the
starter1
branch
from
691de9b
to
23159ad
Compare
server/package.json
Outdated
| "main": "server.js", | ||
| "scripts": { | ||
| "test": "echo \"Error: no test specified\" && exit 1", | ||
| "build": "npx tsc -b" |
There was a problem hiding this comment.
There should be an output path specified build
There was a problem hiding this comment.
I just realized that this script wasn't actually doing anything, and the reason why node was able to run the server was because the Dockerfile was copying server.js straight into the container, tsc actually isn't a valid command since it's part of the devDependencies and isn't installed. Currently, looking into the issue.
There was a problem hiding this comment.
"devDependencies": {
"@types/express": "^4.17.13",
"@types/node": "^16.9.2",
"@types/pg": "^8.6.1",
"nodemon": "^2.0.12",
"typescript": "^4.4.3"
}
I feel like most of these packages should belong in devDependencies, but I don't see a way to resolve the issue other than moving the @types and typescript to dependencies instead. Let me know if I should follow through with this.
There was a problem hiding this comment.
Lets npm install and drop --production for the server builder.
I think it should be safe, otherwise we'll come up with an alternative solution if issues arise
There was a problem hiding this comment.
Just as a note, the docker container size has increased from around 60 MB to 130 MB, but everything seems to be in working order.
- Delete server.js file, emit file to `/build` Compiled .js files should not be in the same directory as server files. Compile and emit to a specific `build` folder instead. - Remove `eject` script from `/client/package.json` This is to prevent accidental ejections which could ruin the user's environment. - Flatten `/build` in `/usr/share/nginx/html/` Update this as to follow typical nginx path convention. - Remove `/api` location block This change will be provisional until we further develop our apis. - Removed unused `package.json` scripts - Update `Dockerfile` to support various changes As `\build` will be flattened, update Dockerfile accordingly. The container will now run the server from the `server/build/server.js` path. As compiling `server.ts` requires the typescript packages, temporarily drop `--production` tag to access required dependencies during `server_build` until alternate solution is needed. Rearrange `EXPOSE` and `RUN` lines in `base`to avoid re-installs and speed up docker build. - Change `heroku.yml` target Heroku should use `base` to build the container application.
- Remove `release` tag from `heroku.yml` This should no longer cause Heroku to be stuck in an infinite loop when building the app. In addition, the `release` dyno is unused and therefore does not need to be included here.
|
Tested the app on Heroku and got these logs. |
|
@gurvirsasan Good catch! I noticed that before actually but it didn't seem to be causing any issues with deploying the actual website. When I tried to use that command, it would actually cause a strange error and actually made things worse, not sure why. Apparently this might be a known bug: |
I can take a look at this and see if I can fix it. I think we may have deprecated packages in our package-lock file |
Alright, make sure you create a separate branch and make a pr to this one. Update: I'm starting to think the issue has to do with the Update 2: Turns out the errors are caused by React having issues with postcss 8, and that they should be releasing the fix soon. In the meantime, the temporary fix suggested by the second link seems to work for now. Going to push this update soon. |
- Add `preinstall` script and `postcss` resolution Errors are caused by known conflict with React and postcss 8 which should have a fix soon according to an closed issue on React's github page. To temporarily fix this issue, allow `npm` to force resolutions with postcss when installing.
- Replace all `npm` commands with `yarn` The reason to do this switch is due to two reasons. The first is that `npm install` completes in 1m30s, while `npm install` completes in 50s, on average. The second is explained in the point below. This runtime difference might appear insignificant, but may improve development time over the long run. - Remove `preinstall` script and change `resolutions` packages The `preinstall` script is misleading and does not run before dependencies are installed, and this is acknowledged in npm/cli#2660 . With the switch to `yarn`, the `preinstall` script becomes obsolete as `yarn` will take care of the `resolutions` without needing a script. In addition, the cause of the security issues was misattributed to the wrong dependency in the previous commit. Remove this and add the true culprits.
There was a problem hiding this comment.
I think you forgot to address the gitignore file issue. It seems to include everything + the kitchen sink. Lets keep it minimal, that makes it easier to read.
It should include
- build output
- node modules
- .vscode directory
- any tmp files/directories
- any other build output
And anything else that may make sense
- Remove unneeded lines in `.gitignore` Make this change to make `.gitignore` easier to read. - Simply `RUN` command in Dockerfile Rather than having two seperate commands, merge the two into one.
heroku starter