nodejs / examples

A repository of runnable Node.js examples that go beyond "hello, world!"
MIT License
649 stars 343 forks source link

express examples #21

Open gireeshpunathil opened 4 years ago

gireeshpunathil commented 4 years ago

Fixes: https://github.com/nodejs/examples/issues/10

/cc @bnb

bnb commented 4 years ago

Will take a more in-depth look soon here (probably Monday) but one tiny piece of feedback is that you're missing README.md for the added directories (see the countEntriesInDirectory CLI example and its parent directory, yargs as an reference for adding both an example and a category)and a GItHub Actions CI file (see .github/workflows/cli-yargs-countentriesindirectory.yml as a reference) for this specific project.

I do need to update the CONTRIBUTING.md to address the CI and testing tooling steps as it currently stands, my bad.

gireeshpunathil commented 4 years ago

@bnb - added a readme, aligning to the scope and purpose of this example. PTAL!

bnb commented 4 years ago

Retargeted to main, will check this as soon as I can - have been slammed with some other priorities recently ❤️

bnb commented 3 years ago

Additionally, we'd want to add a CI file for this project before merging 👍🏻

gireeshpunathil commented 3 years ago

@bnb - thanks! I guess I have addressed most of your comments. In addition,

ideally we'd have this in a directory within the servers/express/ directory - so something like servers/express/basics or servers/express/buffet that gives an example of what's contained within. also change the name property in package.json to match this

moved the content into basics folder, and renamed the app ih the package.json, pls have a look and let me know if that is what you meant.

the license should probably match the repo, which is MIT

done, removed all license content. I guess the one in the repo vortex folder would suffice.

testing locally, the file upload did not work

The /upload assumes there is an uploads folder, and if it is missing, it throws. There are two ways to solve this:

wdyt?

Additionally, we'd want to add a CI file for this project before merging

any pointers? I have never made one.

thanks once again!

bnb commented 3 years ago

The /upload assumes there is an uploads folder, and if it is missing, it throws. There are two ways to solve this:

  • document this
  • pre-create a folder

wdyt?

I'd say both if we can :P

bnb commented 3 years ago

On pointers for CI, I'd say start with the current CI workflow that we have - you basically just need to adapt it:

https://github.com/nodejs/examples/blob/main/.github/workflows/cli-yargs-countentriesindirectory.yaml

There's also the official reference and docs:

https://docs.github.com/en/free-pro-team@latest/actions/reference https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions

I've also written a blog post on GitHub Actions CI that's relatively comprehensive and may be able to help answer any questions:

https://dev.to/bnb/an-unintentionally-comprehensive-introduction-to-github-actions-ci-blm

gireeshpunathil commented 3 years ago

@bnb - I have pushed changes pertinent to out discussion, please take a look!

bnb commented 3 years ago

Seems that tap isn't getting installed which is failing the build. If you could figure out why, that'd be awesome :)

bnb commented 3 years ago

Looks like one of the tests is failing 😅