-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mrc-6023 POC static build #236
base: main
Are you sure you want to change the base?
Changes from 5 commits
6a4a090
182af9a
16e61ec
4a62a0b
373401d
c6048f6
b0b59dc
c84f147
84aeb2a
e41acbf
e97c76a
e901ead
50d66a1
4c7926d
ef39bce
3ee3588
4534546
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
const doc = ` | ||
Usage: | ||
builder <path-to-config> <dest-path> <path-to-mustache-views> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd assumed this would always be done from a development context, but yeah I suppose it doesn't have to be! |
||
`; | ||
|
||
import { docopt } from "docopt"; | ||
import { version } from "../version"; | ||
|
||
export const processArgs = (argv: string[] = process.argv) => { | ||
const opts = docopt(doc, { argv: argv.slice(2), version, exit: false }); | ||
const configPath = opts["<path-to-config>"] as string; | ||
const destPath = opts["<dest-path>"] as string; | ||
const viewsPath = opts["<path-to-mustache-views>"] as string; | ||
return { configPath, destPath, viewsPath }; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
import path from "path"; | ||
import fs from "fs"; | ||
import { ConfigReader } from "../configReader"; | ||
import { AppConfig, WodinConfig } from "../types"; | ||
import { version as wodinVersion } from "../version"; | ||
import { render } from "mustache"; | ||
import { ConfigController } from "../controllers/configController"; | ||
import { AppFileReader } from "../appFileReader"; | ||
import axios from "axios"; | ||
import { processArgs } from "./args"; | ||
|
||
const mkdirForce = (path: string) => { | ||
if (!fs.existsSync(path)) { | ||
fs.mkdirSync(path); | ||
} | ||
}; | ||
|
||
const { configPath, destPath, viewsPath } = processArgs(); | ||
|
||
const pathResolved = path.resolve(configPath); | ||
const configReader = new ConfigReader(pathResolved); | ||
const defaultCodeReader = new AppFileReader(`${pathResolved}/defaultCode`, "R"); | ||
const appHelpReader = new AppFileReader(`${pathResolved}/help`, "md"); | ||
const wodinConfig = configReader.readConfigFile("wodin.config.json") as WodinConfig; | ||
|
||
mkdirForce(destPath); | ||
const appsPathFull = path.resolve(destPath, wodinConfig.appsPath); | ||
if (fs.existsSync(appsPathFull)) { | ||
fs.rmSync(appsPathFull, { recursive: true }); | ||
} | ||
fs.mkdirSync(appsPathFull); | ||
|
||
const sessionId = null; | ||
const shareNotFound = null; | ||
const baseUrl = wodinConfig.baseUrl.replace(/\/$/, ""); | ||
|
||
const appNames = fs.readdirSync(path.resolve(configPath, wodinConfig.appsPath)).map(fileName => { | ||
return /(.*)\.config\.json/.exec(fileName)![1]; | ||
}); | ||
|
||
appNames.forEach(async appName => { | ||
const appNamePath = path.resolve(appsPathFull, appName); | ||
fs.mkdirSync(appNamePath); | ||
|
||
const configWithDefaults = ConfigController.readAppConfigFile( | ||
appName, wodinConfig.appsPath, baseUrl, configReader, defaultCodeReader, appHelpReader | ||
); | ||
const readOnlyConfigWithDefaults = { ...configWithDefaults, readOnlyCode: true }; | ||
const configResponse = { | ||
status: "success", | ||
errors: null, | ||
data: readOnlyConfigWithDefaults | ||
}; | ||
fs.writeFileSync(path.resolve(appNamePath, "config.json"), JSON.stringify(configResponse)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
|
||
const versionsResponse = await axios.get("http://localhost:8001/"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes definitely! |
||
fs.writeFileSync(path.resolve(appNamePath, "versions.json"), JSON.stringify(versionsResponse.data)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
const modelResponse = await axios.post("http://localhost:8001/compile", { | ||
model: defaultCodeReader.readFile(appName), | ||
requirements: { timeType: configWithDefaults.appType === "stochastic" ? "discrete" : "continuous" } | ||
}); | ||
fs.writeFileSync(path.resolve(appNamePath, "modelCode.json"), JSON.stringify(modelResponse.data)); | ||
|
||
|
||
const config = configReader.readConfigFile(wodinConfig.appsPath, `${appName}.config.json`) as AppConfig; | ||
const viewOptions = { | ||
appName, | ||
baseUrl, | ||
appsPath: wodinConfig.appsPath, | ||
appType: config.appType, | ||
title: `${config.title} - ${wodinConfig.courseTitle}`, | ||
appTitle: config.title, | ||
courseTitle: wodinConfig.courseTitle, | ||
wodinVersion, | ||
loadSessionId: sessionId || "", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should always be null, right? Same for shareNotFound. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ooo yes, completely forgot to just change those to null! |
||
shareNotFound: shareNotFound || "", | ||
mathjaxSrc: "https://cdn.jsdelivr.net/npm/mathjax@3/es5/tex-chtml.js", | ||
enableI18n: wodinConfig.enableI18n ?? false, // if option not set then false by default | ||
defaultLanguage: wodinConfig?.defaultLanguage || "en", | ||
hotReload: false | ||
}; | ||
|
||
const mustacheTemplate = fs.readFileSync(path.resolve(viewsPath, "app.mustache")).toString(); | ||
const htmlFile = render(mustacheTemplate, viewOptions); | ||
fs.writeFileSync(path.resolve(appNamePath, "index.html"), htmlFile); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
<template> | ||
<div> | ||
<button | ||
v-if="defaultCodeExists" | ||
v-if="defaultCodeExists && !STATIC_BUILD" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i can see that in this case perhaps, but then places like 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if i were to pull this condition out then i would need to compute
i cant quite think of a better idea yet but i do think the less the app knows about i have however taken out the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 That approach doesnt work for removing the Session stuff, as that is a static build special. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
class="btn btn-primary btn-sm mb-2" | ||
id="reset-btn" | ||
v-help="'resetCode'" | ||
|
@@ -24,6 +24,7 @@ import Timeout = NodeJS.Timeout; | |
import { AppConfig, OdinModelResponse } from "../../types/responseTypes"; | ||
import { CodeAction } from "../../store/code/actions"; | ||
import { CodeMutation } from "../../store/code/mutations"; | ||
import { STATIC_BUILD } from "@/parseEnv"; | ||
|
||
interface DecorationOptions { | ||
range: { | ||
|
@@ -189,7 +190,8 @@ export default defineComponent({ | |
editor, | ||
readOnly, | ||
resetCode, | ||
defaultCodeExists | ||
defaultCodeExists, | ||
STATIC_BUILD | ||
}; | ||
} | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we'll pull this out of the controller when we implement for real.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't have to but it seems odd to use a part of the server controller from the static build script.