Update to ESM modules + Vitest #11
Conversation
Zigr1
requested review from
AdamJHall,
TheOrangePuff,
kai-nguyen-aligent,
nathaniel-cruz-aligent,
pasqualinibruno and
quangdo-aligent
| push: | ||
| branches: | ||
| - production | ||
| - staging |
There was a problem hiding this comment.
@Zigr1 We're trying to move to trunk-based development. Can we make changes in our pipeline so:
- We deploy to
stagingon pushing tomainbranch - We deploy to
productionon creating a Github release. - We will not have
productionorstagingbranches anymore.
There was a problem hiding this comment.
@AdamJHall is this the approach we're taking for appbuilder projects too?
There was a problem hiding this comment.
how it will get the correct set of env vars?
There was a problem hiding this comment.
@Zigr1, Yep I think this is the way forward for all int repos.
Regarding how to get the right env, we might be able to do it like:
environment: ${{ github.event_name == 'release' && 'production' || 'staging' }}
| pull_request: | ||
| branches: | ||
| - staging | ||
| - production |
There was a problem hiding this comment.
We can simplify this so this workflow runs on ALL pull request
| @@ -0,0 +1,65 @@ | |||
| version: 2 | |||
| updates: | |||
There was a problem hiding this comment.
I don't see this file in any other project. Do we have a reason for having this in this repo/template only?
There was a problem hiding this comment.
actually, I took .github and pipeline files from APS project
@AdamJHall could you help answering here?
There was a problem hiding this comment.
Ideally we should have dependabot everywhere, as it makes sure you keep all your dependencies up to date and secure.
There is an issue with the noise it creates on client projects and because it assigns to users based on the CODEOWNERS file.
Since this is a template I would HIGHLY recommend having it here, and assigning to the microservices team via CODEOWNERS so that everything stays up to date.
CC: @TheOrangePuff just to get your thoughts
| import { createContext, useContext, useEffect, useState } from 'react'; | ||
| import { RuntimeScript, type Ims } from '../runtime/RuntimeScript'; | ||
| import { mockIms, mockRuntime } from '../runtime/runtimeMocks'; | ||
| import { RuntimeScript, type Ims } from '../runtime/RuntimeScript.ts'; |
There was a problem hiding this comment.
I thought this should be .js file instead of .ts. How do you think, @Zigr1 ?
package.json
Outdated
| "scripts": { | ||
| "lint": "eslint .", | ||
| "lint:fix": "npm run lint -- --fix", | ||
| "test": "vitest --passWithNoTests --coverage ./test", |
There was a problem hiding this comment.
I think we shouldn't pass this --passWithNoTests here. I would prefer this to be in vite.config.js (with a comment) instead.
README.md
Outdated
| - [ ] Deployment pipeline | ||
| - [ ] Pre-commit hooks | ||
| - [x] Deployment pipeline | ||
| - [x] Pre-commit hooks |
There was a problem hiding this comment.
Did you test the hooks, Greg? I'm not sure if we need a prepare script in package.json or not 🤔
There was a problem hiding this comment.
right, I confused this with appbuilder hooks
nathaniel-cruz-aligent
left a comment
nathaniel-cruz-aligent
left a comment
There was a problem hiding this comment.
Just a minor typo error
package.json
Outdated
| "scripts": { | ||
| "lint": "eslint .", | ||
| "lint:fix": "npm run lint -- --fix", | ||
| "test": "vitest --passWithNoTests --coverage ./test", |
There was a problem hiding this comment.
| "test": "vitest --passWithNoTests --coverage ./test", | |
| "test": "vitest --passWithNoTests --coverage ./tests", |
The test script references ./test but the tests are in a ./tests directory
|
@Zigr1 kindly resolve the conflicts |
4 participants
Summary of changes:
Notes to reviewers
🛈 When you've finished leaving feedback, please add a final comment to the PR tagging the author, letting them know that you have finished leaving feedback