Skip to content
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

Open
wants to merge 7 commits into
base: mrc-6018
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2,182 changes: 709 additions & 1,473 deletions app/server/package-lock.json

Large diffs are not rendered by default.

6 changes: 5 additions & 1 deletion app/server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@
"coverage": "vitest -c ./vitest/vitest.unit.config.mjs run --coverage",
"lint": "eslint .",
"lint:fix": "eslint . --fix",
"genversion": "genversion --es6 --semi --double src/version.ts"
"genversion": "genversion --es6 --semi --double src/version.ts",
"static-serve": "http-server ./public -p 3000",
"copy-static-files": "cp ../../config/index.html ./public && cp -r ../../config/files ./public && cp -r ../../config/help ./public",
"build-static-site": "ts-node --project tsconfig.node.json ./src/static-site-builder/wodinBuilder.ts ../../config ./public ./views && npm run copy-static-files"
},
"devDependencies": {
"@eslint/js": "^9.14.0",
Expand All @@ -28,6 +31,7 @@
"@vitest/coverage-istanbul": "^2.1.4",
"axios-mock-adapter": "^2.1.0",
"eslint": "^9.14.0",
"http-server": "^14.1.1",
"genversion": "^3.2.0",
"nodemon": "^3.1.7",
"supertest": "^7.0.0",
Expand Down
4 changes: 2 additions & 2 deletions app/server/src/controllers/configController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export const configDefaults = (appType: string) => {
};

export class ConfigController {
private static _readAppConfigFile = (
static readAppConfigFile = (
Copy link
Contributor

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.

Copy link
Collaborator Author

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

appName: string,
appsPath: string,
_baseUrl: string,
Expand Down Expand Up @@ -54,7 +54,7 @@ export class ConfigController {
configReader, appsPath, defaultCodeReader, appHelpReader, baseUrl
} = req.app.locals as AppLocals;

const config = this._readAppConfigFile(
const config = this.readAppConfigFile(
appName,
appsPath,
baseUrl,
Expand Down
15 changes: 15 additions & 0 deletions app/server/src/static-site-builder/args.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
const doc = `

Check warning on line 1 in app/server/src/static-site-builder/args.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/args.ts#L1

Added line #L1 was not covered by tests
Usage:
builder <path-to-config> <dest-path> <path-to-mustache-views>
Copy link
Contributor

Choose a reason for hiding this comment

The 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..?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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

`;

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 };

Check warning on line 14 in app/server/src/static-site-builder/args.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/args.ts#L10-L14

Added lines #L10 - L14 were not covered by tests
};
97 changes: 97 additions & 0 deletions app/server/src/static-site-builder/wodinBuilder.ts
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) => {

Check warning on line 12 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L12

Added line #L12 was not covered by tests
if (!fs.existsSync(path)) {
fs.mkdirSync(path);

Check warning on line 14 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L14

Added line #L14 was not covered by tests
}
};

const { configPath, destPath, viewsPath } = processArgs();

Check warning on line 18 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L18

Added line #L18 was not covered by tests

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;

Check warning on line 24 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L20-L24

Added lines #L20 - L24 were not covered by tests

mkdirForce(destPath);
const appsPathFull = path.resolve(destPath, wodinConfig.appsPath);

Check warning on line 27 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L26-L27

Added lines #L26 - L27 were not covered by tests
if (fs.existsSync(appsPathFull)) {
fs.rmSync(appsPathFull, { recursive: true });

Check warning on line 29 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L29

Added line #L29 was not covered by tests
}
fs.mkdirSync(appsPathFull);

Check warning on line 31 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L31

Added line #L31 was not covered by tests

const sessionId = null;
const shareNotFound = null;
const baseUrl = wodinConfig.baseUrl.replace(/\/$/, "");

Check warning on line 35 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L33-L35

Added lines #L33 - L35 were not covered by tests

const appNames = fs.readdirSync(path.resolve(configPath, wodinConfig.appsPath)).map(fileName => {
return /(.*)\.config\.json/.exec(fileName)![1];

Check warning on line 38 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L37-L38

Added lines #L37 - L38 were not covered by tests
});

appNames.forEach(async appName => {
const appNamePath = path.resolve(appsPathFull, appName);
fs.mkdirSync(appNamePath);

Check warning on line 43 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L41-L43

Added lines #L41 - L43 were not covered by tests

const configWithDefaults = ConfigController.readAppConfigFile(

Check warning on line 45 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L45

Added line #L45 was not covered by tests
appName, wodinConfig.appsPath, baseUrl, configReader, defaultCodeReader, appHelpReader
);
const readOnlyConfigWithDefaults = { ...configWithDefaults, readOnlyCode: true };
const configResponse = {

Check warning on line 49 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L48-L49

Added lines #L48 - L49 were not covered by tests
status: "success",
errors: null,
data: readOnlyConfigWithDefaults
};
fs.writeFileSync(path.resolve(appNamePath, "config.json"), JSON.stringify(configResponse));

Check warning on line 54 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L54

Added line #L54 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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/");
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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));

Check warning on line 58 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L57-L58

Added lines #L57 - L58 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The 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. /runner/ode.json. But then it's complicated by being scoped by app.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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));

Check warning on line 62 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L61-L62

Added lines #L61 - L62 were not covered by tests

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));

Check warning on line 66 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L65-L66

Added lines #L65 - L66 were not covered by tests
}
Comment on lines +61 to +67
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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", {

Check warning on line 69 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L69

Added line #L69 was not covered by tests
model: defaultCodeReader.readFile(appName),
requirements: { timeType: configWithDefaults.appType === "stochastic" ? "discrete" : "continuous" }

Check warning on line 71 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L71

Added line #L71 was not covered by tests
});
fs.writeFileSync(path.resolve(appNamePath, "modelCode.json"), JSON.stringify(modelResponse.data));

Check warning on line 73 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L73

Added line #L73 was not covered by tests


const config = configReader.readConfigFile(wodinConfig.appsPath, `${appName}.config.json`) as AppConfig;
const viewOptions = {

Check warning on line 77 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L76-L77

Added lines #L76 - L77 were not covered by tests
appName,
baseUrl,
appsPath: wodinConfig.appsPath,
appType: config.appType,
title: `${config.title} - ${wodinConfig.courseTitle}`,
appTitle: config.title,
courseTitle: wodinConfig.courseTitle,
wodinVersion,
loadSessionId: sessionId || "",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should always be null, right? Same for shareNotFound.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooo yes, completely forgot to just change those to null!

shareNotFound: shareNotFound || "",

Check warning on line 87 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L86-L87

Added lines #L86 - L87 were not covered by tests
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",

Check warning on line 90 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L89-L90

Added lines #L89 - L90 were not covered by tests
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);

Check warning on line 96 in app/server/src/static-site-builder/wodinBuilder.ts

View check run for this annotation

Codecov / codecov/patch

app/server/src/static-site-builder/wodinBuilder.ts#L94-L96

Added lines #L94 - L96 were not covered by tests
});
8 changes: 8 additions & 0 deletions app/static/env.d.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
/// <reference types="vite/client" />

interface ImportMetaEnv {
readonly VITE_STATIC_BUILD: string
}

interface ImportMeta {
readonly env: ImportMetaEnv
}

declare module "vuex" {
export * from "vuex/types/index.d.ts";
export * from "vuex/types/helpers.d.ts";
Expand Down
1 change: 1 addition & 0 deletions app/static/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"dev": "npm run copy-assets && vite",
"build-with-check": "npm run copy-assets && run-p type-check \"build-only {@}\" -- && npm run copy-dist",
"build": "npm run copy-assets && vite build && npm run copy-dist",
"static-build": "npm run copy-assets && VITE_STATIC_BUILD=true vite build && npm run copy-dist",
"copy-assets": "rm -rf ../server/public/* && cp -r ./src/assets/ ../server/public/ && cp ../server/assets/favicon.ico ../server/public/.",
"copy-dist": "cp -r ./dist/. ../server/public/.",
"test:unit": "vitest",
Expand Down
29 changes: 29 additions & 0 deletions app/static/src/apiService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
import { AppCtx } from "./types/utilTypes";
import { ErrorsMutation } from "./store/errors/mutations";
import { AppState } from "./store/appState/state";
import { STATIC_BUILD } from "./parseEnv";

declare let appNameGlobal: string;

export interface ResponseWithType<T> extends ResponseSuccess {
data: T;
Expand Down Expand Up @@ -168,15 +171,41 @@
return `${this._baseUrl}${url}`;
}

private _overrideGetRequestsStaticBuild(url: string) {
let getUrl: string | null = null;

Check warning on line 175 in app/static/src/apiService.ts

View check run for this annotation

Codecov / codecov/patch

app/static/src/apiService.ts#L174-L175

Added lines #L174 - L175 were not covered by tests
if (url === `/config/${appNameGlobal}`) {
getUrl = "./config.json";

Check warning on line 177 in app/static/src/apiService.ts

View check run for this annotation

Codecov / codecov/patch

app/static/src/apiService.ts#L177

Added line #L177 was not covered by tests
} else if (url === "/odin/versions") {
getUrl = "./versions.json";

Check warning on line 179 in app/static/src/apiService.ts

View check run for this annotation

Codecov / codecov/patch

app/static/src/apiService.ts#L179

Added line #L179 was not covered by tests
} else if (url === "/odin/runner/ode") {
getUrl = "./runnerOde.json";

Check warning on line 181 in app/static/src/apiService.ts

View check run for this annotation

Codecov / codecov/patch

app/static/src/apiService.ts#L181

Added line #L181 was not covered by tests
} else if (url === "/odin/runner/discrete") {
getUrl = "./runnerDiscrete.json";

Check warning on line 183 in app/static/src/apiService.ts

View check run for this annotation

Codecov / codecov/patch

app/static/src/apiService.ts#L183

Added line #L183 was not covered by tests
}
if (getUrl === null) return;
return this._handleAxiosResponse(axios.get(getUrl));

Check warning on line 186 in app/static/src/apiService.ts

View check run for this annotation

Codecov / codecov/patch

app/static/src/apiService.ts#L186

Added line #L186 was not covered by tests
}

private _overridePostRequestsStaticBuild(url: string) {
let getUrl: string | null = null;

Check warning on line 190 in app/static/src/apiService.ts

View check run for this annotation

Codecov / codecov/patch

app/static/src/apiService.ts#L189-L190

Added lines #L189 - L190 were not covered by tests
if (url === "/odin/model") {
getUrl = "./modelCode.json";

Check warning on line 192 in app/static/src/apiService.ts

View check run for this annotation

Codecov / codecov/patch

app/static/src/apiService.ts#L192

Added line #L192 was not covered by tests
}
if (getUrl === null) return;
return this._handleAxiosResponse(axios.get(getUrl));

Check warning on line 195 in app/static/src/apiService.ts

View check run for this annotation

Codecov / codecov/patch

app/static/src/apiService.ts#L195

Added line #L195 was not covered by tests
}

async get<T>(url: string): Promise<void | ResponseWithType<T>> {
this._verifyHandlers(url);
if (STATIC_BUILD) return this._overrideGetRequestsStaticBuild(url);
const fullUrl = this._fullUrl(url);

return this._handleAxiosResponse(axios.get(fullUrl));
}

async post<T>(url: string, body: any, contentType = "application/json"): Promise<void | ResponseWithType<T>> {
this._verifyHandlers(url);
if (STATIC_BUILD) return this._overridePostRequestsStaticBuild(url);
const headers = { "Content-Type": contentType };
const fullUrl = this._fullUrl(url);
return this._handleAxiosResponse(axios.post(fullUrl, body, { headers }));
Expand Down
27 changes: 16 additions & 11 deletions app/static/src/components/WodinSession.vue
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import { AppStateGetter } from "../store/appState/getters";
import { SessionMetadata } from "../types/responseTypes";
import { SessionsMutation } from "../store/sessions/mutations";
import { STATIC_BUILD } from "@/parseEnv";

export default defineComponent({
name: "WodinSession",
Expand All @@ -31,7 +32,9 @@
const store = useStore();

const initialised = ref(false);
const appInitialised = computed(() => !!store.state.config && !!store.state.sessions.sessionsMetadata);
const appInitialised = computed(() => STATIC_BUILD ?
!!store.state.config :

Check warning on line 36 in app/static/src/components/WodinSession.vue

View check run for this annotation

Codecov / codecov/patch

app/static/src/components/WodinSession.vue#L36

Added line #L36 was not covered by tests
!!store.state.config && !!store.state.sessions.sessionsMetadata);

// These props won't change as provided by server
const { appName, baseUrl, loadSessionId, appsPath, enableI18n, defaultLanguage } = props;
Expand All @@ -54,16 +57,18 @@
});

watch(appInitialised, () => {
// Child component will either be SessionsPage or WodinApp depending on route - both will need the latest
// session id so delay rendering these until this has been committed
const baseUrlPath = store.getters[AppStateGetter.baseUrlPath];
const sessions = localStorageManager.getSessionIds(store.state.appName, baseUrlPath);
const sessionId = sessions.length ? sessions[0] : null;
// check latest session id is actually available from the back end
const sessionAvailable =
sessionId && !!store.state.sessions.sessionsMetadata.find((s: SessionMetadata) => s.id === sessionId);
if (sessionAvailable) {
store.commit(`sessions/${SessionsMutation.SetLatestSessionId}`, sessionId);
if (!STATIC_BUILD) {
// Child component will either be SessionsPage or WodinApp depending on route - both will need the latest
// session id so delay rendering these until this has been committed
const baseUrlPath = store.getters[AppStateGetter.baseUrlPath];
const sessions = localStorageManager.getSessionIds(store.state.appName, baseUrlPath);
const sessionId = sessions.length ? sessions[0] : null;
// check latest session id is actually available from the back end
const sessionAvailable =
sessionId && !!store.state.sessions.sessionsMetadata.find((s: SessionMetadata) => s.id === sessionId);
if (sessionAvailable) {
store.commit(`sessions/${SessionsMutation.SetLatestSessionId}`, sessionId);
}
}
initialised.value = true;
});
Expand Down
11 changes: 4 additions & 7 deletions app/static/src/components/WodinTabs.vue
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,15 @@
<script lang="ts">
import { defineComponent, ref, PropType } from "vue";

interface Props {
tabNames: string[];
}

export default defineComponent({
name: "WodinTabs",
props: {
tabNames: { type: Array as PropType<string[]>, required: true }
tabNames: { type: Array as PropType<readonly string[]>, required: true },
initSelectedTab: { type: String }
},
emits: ["tabSelected"],
setup(props: Props, { emit }) {
const selectedTabName = ref(props.tabNames[0]);
setup(props, { emit }) {
const selectedTabName = ref(props.initSelectedTab || props.tabNames[0]);

const tabSelected = (tabName: string) => {
selectedTabName.value = tabName;
Expand Down
12 changes: 9 additions & 3 deletions app/static/src/components/basic/BasicApp.vue
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<template>
<wodin-app>
<template v-slot:left>
<wodin-tabs id="left-tabs" :tabNames="['Code', 'Options']">
<wodin-tabs id="left-tabs" :tabNames="leftTabNames" :init-selected-tab="initSelectedTab">
<template v-slot:Code>
<code-tab></code-tab>
</template>
Expand Down Expand Up @@ -30,7 +30,7 @@
</template>

<script lang="ts">
import { defineComponent } from "vue";
import { defineComponent, PropType } from "vue";
import { useStore } from "vuex";
import WodinApp from "../WodinApp.vue";
import WodinTabs from "../WodinTabs.vue";
Expand All @@ -44,6 +44,8 @@ import { VisualisationTab } from "../../store/appState/state";
import HelpTab from "../help/HelpTab.vue";
import includeConfiguredTabs from "../mixins/includeConfiguredTabs";

const leftTabNames = ['Code', 'Options'] as const;

export default defineComponent({
name: "BasicApp",
components: {
Expand All @@ -56,6 +58,9 @@ export default defineComponent({
WodinApp,
WodinTabs
},
props: {
initSelectedTab: { type: String as PropType<typeof leftTabNames[number]>, required: false }
},
setup() {
const store = useStore();
const rightTabSelected = (tab: string) => {
Expand All @@ -70,7 +75,8 @@ export default defineComponent({
rightTabSelected,
helpTabName,
multiSensitivityTabName,
rightTabNames
rightTabNames,
leftTabNames
};
}
});
Expand Down
6 changes: 4 additions & 2 deletions app/static/src/components/code/CodeEditor.vue
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"
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

@M-Kusumgar M-Kusumgar Dec 3, 2024

Choose a reason for hiding this comment

The 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 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,

class="btn btn-primary btn-sm mb-2"
id="reset-btn"
v-help="'resetCode'"
Expand All @@ -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: {
Expand Down Expand Up @@ -189,7 +190,8 @@ export default defineComponent({
editor,
readOnly,
resetCode,
defaultCodeExists
defaultCodeExists,
STATIC_BUILD
};
}
});
Expand Down
Loading
Loading