Mrc 5951 - Address misc comments #218
Conversation
| @@ -133,3 +140,9 @@ Use the `./scripts/run-version.sh` script setting the branch references for the | |||
| ``` | |||
| ./scripts/run-version.sh --app mrc-1234 --api mrc-2345 | |||
| ``` | |||
|
|
|||
| ## Hot Reloading | |||
There was a problem hiding this comment.
Maybe just mention in here what npm command each of these corresponds to as well.
| export const processArgs = (argv: string[] = process.argv) => { | ||
| const opts = docopt(doc, { argv: argv.slice(2), version, exit: false }); | ||
| const path = opts["<path>"] as string; | ||
| const given = { | ||
| baseUrl: opts["--base-url"] as Perhaps<string>, | ||
| odinApi: opts["--odin-api"] as Perhaps<string>, | ||
| redisUrl: opts["--redis-url"] as Perhaps<string>, | ||
| hotReload: opts["--hot-reload"] as Perhaps<string>, | ||
| hotReload: parseArgBoolean(opts["--hot-reload"], "hot-reload"), |
There was a problem hiding this comment.
It looks like hotReload gets passed out of here whether it's null or not, unlike the other args. But it was doing that anyway I think. So null being falsey we just end up with false as hotReload's default I guess?
There was a problem hiding this comment.
the parseArgBoolean can return null, it is the same as the other args no? they will also be passed as null if not specified, but yes null being falsey means our default is hot reloading as false
| @@ -62,7 +62,7 @@ export default defineConfig({ | |||
| * Use the preview server on CI for more realistic testing. | |||
| * Playwright will re-use the local server if there is already a dev-server running. | |||
| */ | |||
| command: process.env.CI ? 'npm run preview' : 'npm run dev', | |||
| command: process.env.CI ? 'npm run build' : 'npm run dev', | |||
There was a problem hiding this comment.
So the server has already been run earlier in the workflow on CI, so we just need to build the front end?
There was a problem hiding this comment.
im actually not 100% sure how we are doing the playwright stuff yet, this is more of an inbetween change because preview builds the production bundle and then serves it but we dont want that in any situation because our express server first needs to inject some variables into it so but changed it to build, the server should be run after the build so that the bundle is copied to the public folder in the server before it starts up
There was a problem hiding this comment.
as a broader point ive also been wondering if its worth just changing to github actions for the browser tests, i think its worth it
done! https://mrc-ide.myjetbrains.com/youtrack/issue/mrc-5966/update-css-src-in-wodin-repos |
This addresses misc comments we made during dicussions. Main changes include:
--hot-reloadoptionapp.hbsview based on ifhotReloadis enabled or not, i didnt switch the js and css srcs explicitly because with chunking in the production file we may need to include more than one script tagnpm run preview(which build for prod and starts a server) tonpm run buildfontawesome.csspointing to the README if you want to add more iconsparseTime->parseDateTimeEventEmitterjust works! but before it was{ EventEmitter } from ...and now itsEventEmitter from ...TokenandRenderertypes frommarkdown-it/index.d.tsPlotDatanow we explicitly define thePlotData.xandPlotData.ytype asnumber[]index.htmlinconfigfolder, before we used to extract outbootstrap.cssby copying that file fromnode_modules/bootstrap. to keep it simpler ive just gone with usingwodin.csswhich has bootstrap in it. This does mean that we need to update theindex.htmlin wodin configs for all of the repos online but theres like 10 repos max so i think it is worth it for the extra simplcity