-
-
Notifications
You must be signed in to change notification settings - Fork 359
App Skeleton using Next.js, Node/Express, React #78
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reviewed your changes, and I'm happy with what I've seen. I only have some feedback to offer re: tsconfig
& tslint
- See comments for more information.
EDIT: Awesome work on this! 💯
tslint.json
Outdated
@@ -0,0 +1,14 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using tslint
or eslint
? Source: Dev.to - Happy to talk about my implementation of eslint
w/ typescript, although the post explains it clearly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have heard tslint will be discontinued soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - That's also mentioned in the post I linked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will be better to use eslint IMHO as like u guys said it will be deprecated soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eslint it is then! I added it as a todo to the PR, does anyone want to make a PR to this branch for setting up eslint? I can probably get to it this weekend but if anyone wants to jump on it I'd be more than happy to hand over the reins 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tslint is now eslint - let me know what you guys think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Nice work! What do you think about using yarn instead of npm? So we leave the door open to use a monorepo with yarn workspaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
body-parser aren't needed anymore it already on express 4.x
Could we maybe use ts-node-dev, it does the same thing but looks nicer (no huge nodemon config), just ts-node-dev index.ts |
Thanks! :) Can you tell me more about the advantages of yarn over npm? From what I understand, npm has caught up significantly over the past few years to the point where they're pretty comparable, but I am also not well versed in this either. |
Yarn workspaces and offline installs are something's that npm doesn't have and that are pretty useful. Also for lazy people you can do |
Good work @timmyichen. There's a minor issue in that the |
Whoops, thanks! I was a bit too hasty with fixing those merge conflicts 🙃 and PRs to my branch are most welcome, yes! |
version: '3' | ||
services: | ||
app: | ||
image: node:10-alpine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the "end product" of this project is a Docker container, how about geting started on creating a Dockerfile for it - instead of defining configurations within Docker Compose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(copied from discord)
docker-compose will really shine when we add additional images, e.g. for postgres or redis or any other service we might need. i think dockerfiles are really only necessary if we're adding special config to a base image (at the moment the base node10 image should be sufficient for what we need)
but also, a dockerfile is probably a good idea for setup including npm install
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a dockerfile is probably a good idea for setup including npm install
Yes, in future we will add many dependencies which then require npm install
. Can you please go ahead and create dockerfile
and link to Docker Compose
Seeing this error with
Anyone else seeing this? Maybe I missed something, not super familiar with JavaScript stuff. edit: nvm |
This reverts commit 8075dc4.
I made a bad commit - just reverted it - will debug tonight - my bad! |
…EADME"" This reverts commit cf502e5.
* chore: added speccy to docker compose dev environment * chore: added speccy to dev deps, added/updated scripts, fixed husky warning * test: added test for SomeComponent * docs: updated scripts and added testing * chore: setup jest testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good IMO.
I think we are not going to use Typescript, isn't? |
@wuilliam321 We're still discussing it, but the current concusses is that we'll use JS in TS file for now, so we get the tooling and a bit of type safety, so that we have a balance of something that looks like JS and tooling associated with TS |
docker-compose.yml
Outdated
@@ -0,0 +1,20 @@ | |||
version: '3' | |||
networks: | |||
chapter: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't necessarily need to specify a network
like this, by default Compose sets up a single network.
$ docker-compose up
WARNING: Some networks were defined but are not used by any service: chapter
speccy: | ||
image: node:10-alpine | ||
command: 'npm run speccy:watch' | ||
volumes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend creating a single top-level volume
and having it referenced in the services, instead of declaring it twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
|
||
### Installing Docker | ||
|
||
Click [here](https://docs.docker.com/v17.12/install/) for the Docker installation site. Scroll down to "Supported Platforms" and follow the instructions to download & install Docker Desktop for your operating system (or Docker CE for linux). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Click [here](https://docs.docker.com/v17.12/install/) for the Docker installation site. Scroll down to "Supported Platforms" and follow the instructions to download & install Docker Desktop for your operating system (or Docker CE for linux). | |
Click [here](https://docs.docker.com/install/#supported-platforms) for the Docker installation site and follow the instructions to download & install Docker Desktop for your operating system (or Docker CE for linux). |
17.12 is outdated, 19.03 is the current version. This link ☝️ always goes to the latest.
docker-compose.yml
Outdated
command: 'npm run dev' | ||
working_dir: /usr/chapter/ | ||
volumes: | ||
- data:/usr/chapter/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- data:/usr/chapter/ | |
- data:/usr/chapter |
Styling - matching the path of the other service's volume
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone ahead and removed all remaining tailing slashed in PR his original pr
version: '3' | ||
services: | ||
app: | ||
image: node:10-alpine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a dockerfile is probably a good idea for setup including npm install
Yes, in future we will add many dependencies which then require npm install
. Can you please go ahead and create dockerfile
and link to Docker Compose
* fix(setup) Add note for linux users about docker-compose problem with PWD * fix(client) Use named functions for components * fix(setup) Turn of ts noImplicitAny * fix(setup) Use es6 imports in next config * fix(docker) Remove top level volumes * fix(setup) add newline to end of package.json
Update README.md
).master
branch of Chapter.This PR adds the basic skeleton of the app using the following technologies (mentioned in the stack conversation #2 :
In addition:
Some other things it does that I enjoy but is a personal taste and is also RFC:
client
andserver
to avoid../../..
in the codebaseexpress-async-handler
andexpress-response-errors
for better async support and error handling in express routesMisc notes:
package.json
, let me know if those should be changed (e.g. version)TODO:
express-response-errors
Please let me know your thoughts!