mrc-6023 POC static build #236
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #236 +/- ##
==========================================
- Coverage 99.77% 97.67% -2.10%
==========================================
Files 183 189 +6
Lines 4463 4563 +100
Branches 990 1005 +15
==========================================
+ Hits 4453 4457 +4
- Misses 9 98 +89
- Partials 1 8 +7 ☔ View full report in Codecov by Sentry. |
Wasn't there talk of doing this in a github action at one point? So user wouldn't actually need to build on their local machine. And could then publish direct to ghp potentially. But maybe I'm misremembering. |
yep i definitely forgot a bit about that, that just solves the whole issue, we can just setup the env however we want in our custom action |
| @@ -23,7 +23,7 @@ export const configDefaults = (appType: string) => { | |||
| }; | |||
|
|
|||
| export class ConfigController { | |||
| private static _readAppConfigFile = ( | |||
| static readAppConfigFile = ( | |||
There was a problem hiding this comment.
I guess we'll pull this out of the controller when we implement for real.
There was a problem hiding this comment.
actually we dont have to! we need the whole config controller any and tree shaking means that we only get that and not everything else, we just put wodinBuilder as the entry point
There was a problem hiding this comment.
we don't have to but it seems odd to use a part of the server controller from the static build script.
| @@ -0,0 +1,15 @@ | |||
| const doc = ` | |||
| Usage: | |||
| builder <path-to-config> <dest-path> <path-to-mustache-views> | |||
There was a problem hiding this comment.
Yeah. the views path doesn't seem like it needs to be a parameter as it's not something that should change per build..?
There was a problem hiding this comment.
it can change between development and production, it depends on the folder structure of the dist folder, which may not be the same as the folder structure of our app/server directory, i guess we can also force them to be the same and hardcode that path in wodin builder as well but felt nice to give that flexibility
There was a problem hiding this comment.
I'd assumed this would always be done from a development context, but yeah I suppose it doesn't have to be!
| appTitle: config.title, | ||
| courseTitle: wodinConfig.courseTitle, | ||
| wodinVersion, | ||
| loadSessionId: sessionId || "", |
There was a problem hiding this comment.
This should always be null, right? Same for shareNotFound.
There was a problem hiding this comment.
ooo yes, completely forgot to just change those to null!
| errors: null, | ||
| data: readOnlyConfigWithDefaults | ||
| }; | ||
| fs.writeFileSync(path.resolve(appNamePath, "config.json"), JSON.stringify(configResponse)); |
There was a problem hiding this comment.
So it's writing out a pre-canned response file, status and all, for config etc, which will be read by the front end rather than talking to the server? I'd assumed that the apiService in the front end would, when configured as static, just read this data directly from the public path of the site, but I guess it makes the code simpler if it assumes that all these "responses" are going to be in the same format as for the dynamic site. I see you're doing a similar thing for the version and runner responses, just piping the the response direct to file.
There was a problem hiding this comment.
yep exactly i went back and forth in terms of how to get these responses, also considered them just being javascript files or something like that, all the other options just required a bit more code change which isnt a problem but this seemed the neatest to me, its literally just a fake local api but yh all logic works exactly the same, we dont need to do any extra processing of the response or anything like that
i feel like the more "branches" we add with these two modes diverging the more maintenance we add
| fs.writeFileSync(path.resolve(appNamePath, "config.json"), JSON.stringify(configResponse)); | ||
|
|
||
|
|
||
| const versionsResponse = await axios.get("http://localhost:8001/"); |
There was a problem hiding this comment.
I guess for the full implementation we'll make the api path configurable, deal with error handling etc.
There was a problem hiding this comment.
yes definitely!
|
|
||
|
|
||
| const versionsResponse = await axios.get("http://localhost:8001/"); | ||
| fs.writeFileSync(path.resolve(appNamePath, "versions.json"), JSON.stringify(versionsResponse.data)); |
There was a problem hiding this comment.
I wonder if we could make the mappings between the api paths and the file names a bit less arbitrary. So if the path to the json file was always the same as the url in the backend that is called in the dynamic app e.g. /runner/ode.json. But then it's complicated by being scoped by app.
There was a problem hiding this comment.
sure yh i dont think having folders per app is that bad personally if we want to keep them consistent with the api urls
| const runnerResponse = await axios.get("http://localhost:8001/support/runner-ode"); | ||
| fs.writeFileSync(path.resolve(appNamePath, "runnerOde.json"), JSON.stringify(runnerResponse.data)); | ||
|
|
||
| if (configWithDefaults.appType === "stochastic") { | ||
| const runnerResponse = await axios.get("http://localhost:8001/support/runner-discrete"); | ||
| fs.writeFileSync(path.resolve(appNamePath, "runnerDiscrete.json"), JSON.stringify(runnerResponse.data)); | ||
| } |
There was a problem hiding this comment.
These responses are identical for every model aren't they? so I guess they don't need to be fetched or even saved per app?
There was a problem hiding this comment.
they are! the reason i did it per app is the request fired is relative to the app, so like apps/day1/runner/ode or something like that, so it was easier to have a duplicate in each app, obviously this doesnt scale well but i dont think people ever have more than 5-6 apps, so that much duplication of that file seemed alright for slightly simpler code but happy to change it if needed
| @@ -1,7 +1,7 @@ | |||
| <template> | |||
| <div> | |||
| <button | |||
| v-if="defaultCodeExists" | |||
| v-if="defaultCodeExists && !STATIC_BUILD" | |||
There was a problem hiding this comment.
I feel like it would maybe be a bit nicer if the components themselves didn't know directly about STATIC_BUILD and read a getter from the store to say if code should be resettable etc.
There was a problem hiding this comment.
i can see that in this case perhaps, but then places like WodinSession.vue we have code in a watcher that relies on the static build variable, in BasicApp.vue and other app types we have a switch on whether "Options" is the tab shown initially or not, and in some places we just have a v-if on only the static build variable
so perhaps in the app types we can have shared code, either a getter or a function that tells us what the initial tab is, but for a v-if on just the static build variable im not sure a getter or a function does anything for us, itll just wrap static build in another layer
yh im just not sure how we have getters that cover all these slightly different ways of using static build without basically creating a different getter for each of these use cases, at which point we may as well use it in the component
There was a problem hiding this comment.
yeah, fair point. But as you say for the lower level components like this one, maybe it should just be a prop.
There was a problem hiding this comment.
if i were to pull this condition out then i would need to compute defaultCodeExists in the parent component this in itself is not a problem, but the question is, should this component still be responsible for resetting the code (it currently resets the code itself)?
- on one hand it feels weird to toggle visibility of the reset button in the parents and then the child control the actual action of resetting
- on the other hand i think it would be messier to pull out the functionality of resetting the code from this component because it needs the editor instance
i cant quite think of a better idea yet but i do think the less the app knows about STATIC_BUILD the better for us!
i have however taken out the STATIC_BUILD from BasicApp.vue, FitApp.vue and StochasticApp.vue and used the prop idea there! because i could concentrate that logic in the router.ts file,
There was a problem hiding this comment.
I suppose one thing you could do would be to have something at the top level of the store or app be opinionated about how STATIC_BUILD being true should impact how the app should interpret AppConfig e.g. it could set readOnlyCode to true even if wasn't defined as such in the config it loaded. So then the components don't care that it's a static build, as far as they're concerned it's just any read only app.
That approach doesnt work for removing the Session stuff, as that is a static build special.
There was a problem hiding this comment.
hmmm thats a good idea actually! readOnlyCode quirks will be solved! yh think the apiService one, sessions page one and wodinSession ones will probably remain, but thats not too bad!
app/static/src/router.ts
Outdated
| @@ -31,7 +34,7 @@ export function initialiseRouter( | |||
| component: appComponent, | |||
| props: { | |||
| initSelectedTab: STATIC_BUILD ? "Options" : undefined | |||
| } as Parameters<NonNullable<(typeof appComponent)["setup"]>>["0"] | |||
| } as Parameters<NonNullable<IntersectionAppComponent["setup"]>>["0"] | |||
There was a problem hiding this comment.
this line basically gets the type of the first argument of setup function (which is props), so we can be sure that the selected tab exists for every app type
There was a problem hiding this comment.
I can see why you've done this but it's quite confusing to read! I don't really love invoking the first parameter of setup as a way of getting out of declaring actual props types - it does save you that maintenance overhead but with something of a loss of clarity! Could we at least declare this type above and put a comment on it?
There was a problem hiding this comment.
sure yh, sadly i couldnt actually find a way to extract the prop types any other way from defineComponent type which sucks, feel like they couldve made that easier
There was a problem hiding this comment.
Yeah, agreed. We've usually just defined the types ourselves in the past. It's boring boilerplate, but it's easy to comprehend...
ignore the merge conflicts and test failures! just a POC
Testing
npm ci --prefix=app/staticandnpm ci --prefix=app/serverto get all updated packagesodin.apicontainer so run./scripts/run-dev-dependencies.sh(dont needredisbut just using the normal script for now)./scripts/build-and-serve-static-site.shand have fun, itll be on localhost:3000General approach
Network requests
We want to decouple the frontend from the express backend, we can just ignore all the sessions stuff in the app as far as I could tell which is the majority of the work. This leaves the initial setup which comprises of getting:
everything except the config is fetched from
odin.api(for now treating the code in "defaultCode" folder as the static code) so we can do exactly that, we can prefetch from the container that we start up on the users machine these responses and just save them as a json file that can be shipped with the static site. these responses can now be fetched with the overrides in theapiService.tsfile (_overrideGetRequestsStaticBuildand_overridePostRequestsStaticBuildmethods)as for the config, we can reuse the handy
ConfigControllerto inject the config with the defaults (exactly in the same way the server does) with a tiny change, we always keepreadOnlyCodeastruesince this is our static build and we save this as a static json file too same as other requestswe simply ignore all other requests the app makes
Frontend
The frontend takes in an env arg
VITE_STATIC_BUILDto switch between the two build types, main differences you may see are things like not checking session before initialising the app, no sessions dropdown, no "Reset" or "Compile" code buttons, the default tab to start with for users being "Options" instead of the "Code" tab because the code doesnt changeHow does this translate to users workflow?
We publish a static wodin builder package that includes:
wodinBuilder.tsits only a couple of tiny files from theapp/server)The user basically creates the same config as before for a wodin site (perhaps we can change "defaultCode" folder to just "code" folder but perhaps not since users will already have wodin configs ready and can just reuse existing ones if they wish)
wodinBuilderscript takes in config path and destination path as args (views path arg is just for my convenience when i was testing) and so the user can tell us where the config lies and where their public folder is. we can also exec some docker commands from that script to run theodin.apicontainer on their machines, and then the builder has everything it needs to generate out the whole wodin websiteSome final thoughts
The outlined approach above requires the user to have both docker and node installed on their machines. Some other approaches we can consider: