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

Express template #176

Merged
merged 16 commits into from
Aug 19, 2020
Merged

Express template #176

merged 16 commits into from
Aug 19, 2020

Conversation

mieshalannair
Copy link
Contributor

Introducing a simple Hello World expressjs template for skuba.

@mieshalannair mieshalannair requested a review from a team as a code owner August 18, 2020 04:57
@changeset-bot
Copy link

changeset-bot bot commented Aug 18, 2020

🦋 Changeset is good to go

Latest commit: 4dc961e

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

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

This looks like a promising start. I think it would be good to provide a bit more structure in the stencil so those who are starting from scratch have something to go off of.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/cli/init/prompts.ts Outdated Show resolved Hide resolved
template/express-rest-api/Dockerfile Outdated Show resolved Hide resolved
template/express-rest-api/src/main.ts Outdated Show resolved Hide resolved
template/express-rest-api/src/main.ts Outdated Show resolved Hide resolved
Comment on lines 25 to 30
},
"skuba": {
"entryPoint": "src/main.ts",
"template": "koa-rest-api",
"type": "application",
"version": "3.7.7"
}
Copy link
Member

Choose a reason for hiding this comment

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

This stuff is generated

Suggested change
},
"skuba": {
"entryPoint": "src/main.ts",
"template": "koa-rest-api",
"type": "application",
"version": "3.7.7"
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, if i get you correctly. Should I remove this line ? 😕
How about the entry point ?

Copy link
Member

Choose a reason for hiding this comment

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

The whole skuba section can be removed, you'll notice that none of the other templates have it. It's generated dynamically in part from the skuba.template.js, e.g.

https://github.com/mieshalannair/skuba/blob/express-template/template/express-rest-api/skuba.template.js#L6

I believe that specified entry point is mismatched with your template. Is there a reason to deviate and choose main.ts when every other template uses app.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted with thanks. Will make the changes.
main.ts is usually used in the nestjs framework, but I think app.ts will do.

template/express-rest-api/package.json Outdated Show resolved Hide resolved
ENV PORT ${PORT}
EXPOSE ${PORT}

CMD node lib/main
Copy link
Member

Choose a reason for hiding this comment

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

Is this what you run in production? Granted I wouldn't expect these frameworks to properly crash often, but spinning up a new ECS task would be slower than having a clustering implementation spawn another process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we are running our some of our project like this in production. Do you recommend running node with pm2-runtime ?

Copy link
Member

Choose a reason for hiding this comment

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

I think there's an open question for us to resolve around when multiple processes make sense with constrained compute in Fargate. I'm happy to leave this for now.

Choose a reason for hiding this comment

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

Hi @72636c , we did use pm2 for one of our services in the past, but stopped. Main reason: we don't expect crashes to be a part of a normal application lifecycle. It also was causing us some issues, when app was keep crashing but we thought it was running (but probably because of the way, how we configured our monitors in DataDog).

I think there's an open question for us to resolve around when multiple processes make sense with constrained compute in Fargate.

I see this question is more like an advanced topic, which requires to have proper circumstances. We don't have it yet. So I would agree with you that we might leave it out of scope for now.

p.s. on a personal note I would like to hear when and how you use pm2 to overcome "constrained compute in Fargate".

Copy link
Member

@72636c 72636c left a comment

Choose a reason for hiding this comment

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

🚢 thanks for this!

@72636c 72636c changed the title expressjs template Express template Aug 19, 2020
@72636c 72636c merged commit 1f8de87 into seek-oss:master Aug 19, 2020
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.

None yet

3 participants