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

Deployment Scripts and (partial) rewrite of Backend Code #74

Merged
merged 6 commits into from
Dec 1, 2022

Conversation

jackcoberman
Copy link
Contributor

test pr

@wihobbs wihobbs temporarily deployed to bookstore-app November 30, 2022 04:35 Inactive
@wihobbs wihobbs temporarily deployed to bookstore-app November 30, 2022 04:40 Inactive
@wihobbs wihobbs temporarily deployed to bookstore-app November 30, 2022 04:50 Inactive
@wihobbs wihobbs temporarily deployed to bookstore-app November 30, 2022 04:55 Inactive
@wihobbs wihobbs changed the base branch from JO-14 to main November 30, 2022 05:36
@wihobbs wihobbs changed the title Jo 14 deployment Deployment Scripts and (partial) rewrite of Backend Code Nov 30, 2022
@wihobbs
Copy link
Collaborator

wihobbs commented Nov 30, 2022

This is ready for a review sometime tomorrow but is already rolled to deployment (to show that it works).

Important note: we removed the client package-lock.json (and maybe the root dir too?) to get Heroku to deploy (as suggested on their support website.)

console.log(path.join(__dirname, 'client/build'));
app.use(express.static(path.join(__dirname, 'client/build')));
app.get('*', (req,res) => {
res.sendFile(path.resolve(__dirname, 'client', 'build', 'index.html'));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it supposed to be index.html for our purposes?

@jackcoberman jackcoberman marked this pull request as draft November 30, 2022 05:53
@jackcoberman
Copy link
Contributor Author

@wihobbs Currently getting 404 errors when running this in local development - also we need to fix readme to reflect both ".env" and that we are now running "npm run dev" instead of "npm start"

@wihobbs wihobbs marked this pull request as ready for review November 30, 2022 12:15
@wihobbs
Copy link
Collaborator

wihobbs commented Nov 30, 2022

@jackcoberman Are you sure you're navigating to the correct port on your local device? We changed that a few times and I believe 3001 is the one we settled on, but just want to be sure.

I haven't seen that 404 error before -- if you can shoot me a screenshot I'll look into it more.

@wihobbs
Copy link
Collaborator

wihobbs commented Nov 30, 2022

I think for now we should deprecate npm run dev. A new workflow would be:

Build the code -- cd client && npm run build
Go back to the root directory and npm start

This will start the server and the client, and they can communicate with each other (on my machine and our deployment).

Goes without saying that you need npm install in root, backend, and client directories. These are things we need to add to our Readme.

Copy link
Contributor

@rahulbulusu rahulbulusu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good everything works

@jackcoberman jackcoberman merged commit 19a09fb into main Dec 1, 2022
@rahulbulusu rahulbulusu deleted the JO-14-deployment branch January 29, 2023 21:23
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