🛠️ Feat/1 Setup Repo

demariadaniel commented 4 weeks ago

Creates basic React UI, Express API, and Types package, using Vite for set up
Set up unit testing framework using Node Test Runner
Adds basic README docs

ciaranschutte commented 2 weeks ago

looks good! I'd recommend adding prettier + husky at this point. standard in our stack. husky for pre commit checks and prettier for consistent styling prettier-organise-imports package is useful too

ciaranschutte commented 2 weeks ago

license could be added also at this point. kinda thing that usually gets forgotten later!

ciaranschutte commented 1 week ago

A few comments/bits of advice. Overall looking good :thumbs-up: I'll leave it up to you & others on the project to decide how actionable these are.

env
https://github.com/Pan-Canadian-Genome-Library/daco/pull/7/commits/9a14e7c1da573a4019df438feef510532ea0ff2c .env support added here, but no .env.schema added which is helpful to give developers are starting point.

right now the dev script is using tsx with an env flag. for prod, looks like vite should be used, so you'll likely have to use something like https://www.npmjs.com/package/dotenv to read env vars.
no need to export server, I found that confusing. app.listen is the magic here.
it's easier to give prettier a specific folder like src to modify rather than have a tonne of ignore options. When people add other files, they may have to add to prettier ignore. aim is sensible default behaviour.

I don't understand the vite.config.js usage for server. I don't see it in the Vite boilerplates. I think you may be basing it off the library config I shared with you for iobio. In iobio the end goal was a package, a library to install and use in a project.

It's different here. We don't install the server or the ui as packages, so we don't need to build them as libraries. The simplest server setup is to install dependencies then run the server (vite). I believe no additional config needed... just run vite.
app.use(cors()) is setting up middleware cors for all requests on app level, no need to use it later on individual handlers if you set it up like this.

demariadaniel commented 1 week ago

it's easier to give prettier a specific folder like src to modify rather than have a tonne of ignore options. When people add other files, they may have to add to prettier ignore. aim is sensible default behaviour.

Good call, updated npm script to target ./packages & ./apps

demariadaniel commented 1 week ago

I don't understand the vite.config.js usage for server. I don't see it in the Vite boilerplates. I think you may be basing it off the library config I shared with you for iobio. In iobio the end goal was a package, a library to install and use in a project. It's different here. We don't install the server or the ui as packages, so we don't need to build them as libraries. The simplest server setup is to install dependencies then run the server (vite). I believe no additional config needed... just run vite.

So given we don't need to build a package, I don't know if we need Vite at all for the API. I used Vite for setup but it's actually just a Node/Express app. We could make a run:prod script which essentially does the same thing as run:dev at this stage. This is essentially how platform-api is built: https://github.com/icgc-argo/platform-api/blob/develop/package.json#L15

right now the dev script is using tsx with an env flag. for prod, looks like vite should be used, so you'll likely have to use something like https://www.npmjs.com/package/dotenv to read env vars.

The flag is a native Node flag: https://nodejs.org/en/learn/command-line/how-to-read-environment-variables-from-nodejs https://tsx.is/node-enhancement#swap-node-for-tsx I wanted to leverage modern Node features as much as possible to keep it minimal. See above comment, if Vite is not a requirement then is DotEnv needed?

demariadaniel commented 6 days ago

Looks good. Approving.

Not sure why there's a docs folder with empty files. It's a nice thought but unless it's definitely going to worked it's just adding empty files to a repo. if keeping , might be worth adding some content even if very minimal for now.

Thanks @ciaranschutte there's a handful of Readme items to resolve so I created a follow up ticket: https://app.zenhub.com/workspaces/pcgl---pancanadian-genome-library-659eed8bc4fe0c04890cff77/issues/gh/pan-canadian-genome-library/daco/14

These are template files from Overture, will revisit

Pan-Canadian-Genome-Library / daco

🛠️ Feat/1 Setup Repo #7