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.

Sign up for GitHub

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

kiosk: persist built game js in local storage #9726

Merged
merged 2 commits into from
Oct 18, 2023
Merged

kiosk: persist built game js in local storage #9726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22ee5a1
kiosk: persist built game js in local storage
eanders-ms Oct 17, 2023
8a67f09
better null check
eanders-ms Oct 17, 2023
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 changes: 1 addition & 1 deletion kiosk/src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ function App() {
// TODO: Handle this better
dispatch(Actions.setTargetConfig({}));
}
});
});
// Init subsystems.
SimHost.initialize();
NotificationService.initialize();
Expand Down
24 changes: 19 additions & 5 deletions kiosk/src/Components/PlayingGame.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ import { AppStateContext } from "../State/AppStateContext";
import { escapeGame } from "../Transforms/escapeGame";
import { playSoundEffect } from "../Services/SoundEffectService";
import { useOnControlPress } from "../Hooks";
import { stringifyQueryString } from "../Utils";
import * as GamepadManager from "../Services/GamepadManager";
import * as Storage from "../Services/LocalStorage";
import configData from "../config.json";

export default function PlayingGame() {
Expand All @@ -23,11 +25,23 @@ export default function PlayingGame() {

const playUrl = useMemo(() => {
if (gameId) {
const playUrlBase = `${configData.PlayUrlRoot}?id=${gameId}&hideSimButtons=1&noFooter=1&single=1&fullscreen=1&autofocus=1`;
const playQueryParam = kiosk.builtGamesCache[gameId]
? "&server=1"
: "&sendBuilt=1";
return playUrlBase + playQueryParam;
const builtGame = Storage.getBuiltJsInfo(gameId);
return stringifyQueryString(configData.PlayUrlRoot, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I like this change! It makes it a lot clearer what the url is doing.

id: gameId,
// TODO: Show sim buttons on mobile & touch devices.
hideSimButtons: 1,
noFooter: 1,
single: 1,
fullscreen: 1,
autofocus: 1,
// If we have the built game cached, we will send it to the
// simulator once it loads. The `server` flag inhibits the
// simulator from trying to build it.
server: builtGame ? 1 : undefined,
// If we don't have the built game cached, tell the simulator to
// send it to us once it's built and we'll cache it.
sendBuilt: builtGame ? undefined : 1,
});
}
}, [gameId]);

Expand Down
39 changes: 36 additions & 3 deletions kiosk/src/Services/LocalStorage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,17 @@ function delValue(key: string) {
localStorage.removeItem(key);
}

function matchingKeys(pattern: RegExp): string[] {
const keys: string[] = [];
for (let i = 0; i < localStorage.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't clear to me what this function is used for, could you clarify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function returns all local storage keys matching a regex pattern. It is called by the clearBuiltJsInfo method (later in this file) to get all the keys it should delete.

const key = localStorage.key(i);
if (key && pattern.test(key)) {
keys.push(key);
}
}
return keys;
}

function getJsonValue<T>(key: string, defaultValue?: T): T | undefined {
var value = getValue(key);
if (value) {
Expand Down Expand Up @@ -109,10 +120,29 @@ function clearKioskCode() {
delValue(Constants.legacy_kioskCodeExpirationStorageKey);
}

function getBuiltJsInfo(gameId: string): ts.pxtc.BuiltSimJsInfo | undefined {
const ver = pxt.appTarget?.versions?.target;
if (!ver) return undefined;
const key = `builtjs:${ver}:${gameId}`;
const rec = getJsonValue<ts.pxtc.BuiltSimJsInfo>(key);
return rec;
}

function setBuiltJsInfo(gameId: string, builtJs: ts.pxtc.BuiltSimJsInfo) {
const ver = pxt.appTarget?.versions?.target;
if (!ver) return;
const key = `builtjs:${ver}:${gameId}`;
setJsonValue(key, builtJs);
}

function clearBuiltJsInfo() {
const keys = matchingKeys(/^builtjs:/);
for (const key of keys) {
delValue(key);
}
}

export {
getValue,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these no longer used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to restrict the use of these calls to only this file, to ensure that new code would follow the pattern of adding bespoke apis for the specific fields being persisted. Exposing the low-level apis can lead to unnecessarily complicated code elsewhere.

setValue,
delValue,
getJsonValue,
setJsonValue,
getUserAddedGames,
Expand All @@ -123,4 +153,7 @@ export {
setKioskCode,
getKioskCode,
clearKioskCode,
getBuiltJsInfo,
setBuiltJsInfo,
clearBuiltJsInfo,
};
19 changes: 5 additions & 14 deletions kiosk/src/Services/SimHostService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { stateAndDispatch, getBuiltGame } from "../State";
import { BuiltSimJSInfo } from "../Types";
import { stateAndDispatch } from "../State";
import * as Actions from "../State/Actions";
import * as Constants from "../Constants";
import { gameOver } from "../Transforms/gameOver";
import { resetHighScores } from "../Transforms/resetHighScores";
import * as GamepadManager from "./GamepadManager";
import { postNotification } from "../Transforms/postNotification";
import { makeNotification } from "../Utils";
import * as Storage from "./LocalStorage";

export function initialize() {
let controlStates: GamepadManager.ControlStates = {
Expand Down Expand Up @@ -64,8 +64,7 @@ export function initialize() {
GamepadManager.addKeyupListener(keyuphandler);

function sendBuiltGame(gameId: string) {
const { state } = stateAndDispatch();
const builtGame = state.builtGamesCache[gameId];
const builtGame = Storage.getBuiltJsInfo(gameId);
if (builtGame) {
const simIframe = document.getElementsByTagName(
"iframe"
Expand All @@ -80,15 +79,7 @@ export function initialize() {
window.addEventListener("message", event => {
const { state, dispatch } = stateAndDispatch();
if (event.data?.js && state.launchedGameId) {
const builtGame: BuiltSimJSInfo =
state.builtGamesCache?.[state.launchedGameId];
if (!builtGame) {
dispatch(
Actions.addBuiltGame(state.launchedGameId, event.data)
);
} else {
sendBuiltGame(state.launchedGameId);
}
Storage.setBuiltJsInfo(state.launchedGameId, event.data);
}
switch (event.data.type) {
case "simulator":
Expand Down Expand Up @@ -120,7 +111,7 @@ export function initialize() {
break;

case "ready":
const builtGame = getBuiltGame(state.launchedGameId);
const builtGame = Storage.getBuiltJsInfo(state.launchedGameId!);
if (builtGame) {
sendBuiltGame(state.launchedGameId!);
}
Expand Down
16 changes: 0 additions & 16 deletions kiosk/src/State/Actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import {
GameData,
BuiltSimJSInfo,
KioskState,
HighScores,
AllHighScores,
NotificationWithId,
HighScoreWithId,
Expand Down Expand Up @@ -33,12 +31,6 @@ type SetKioskState = ActionBase & {
state: KioskState;
};

type AddBuiltGame = ActionBase & {
type: "ADD_BUILT_GAME";
gameId: string;
info: BuiltSimJSInfo;
};

type SetLaunchedGame = ActionBase & {
type: "SET_LAUNCHED_GAME";
gameId: string;
Expand Down Expand Up @@ -139,7 +131,6 @@ export type Action =
| SetGameList
| SetSelectedGameId
| SetKioskState
| AddBuiltGame
| SetLaunchedGame
| SetLockedGame
| SetAllHighScores
Expand Down Expand Up @@ -179,12 +170,6 @@ const setKioskState = (state: KioskState): SetKioskState => ({
state,
});

const addBuiltGame = (gameId: string, info: BuiltSimJSInfo): AddBuiltGame => ({
type: "ADD_BUILT_GAME",
gameId,
info,
});

const setLaunchedGame = (gameId: string): SetLaunchedGame => ({
type: "SET_LAUNCHED_GAME",
gameId,
Expand Down Expand Up @@ -288,7 +273,6 @@ export {
setGameList,
setSelectedGameId,
setKioskState,
addBuiltGame,
setLaunchedGame,
setLockedGame,
setAllHighScores,
Expand Down
9 changes: 0 additions & 9 deletions kiosk/src/State/Reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,6 @@ export default function reducer(state: AppState, action: Action): AppState {
kioskState: action.state,
};
}
case "ADD_BUILT_GAME": {
return {
...state,
builtGamesCache: {
...state.builtGamesCache,
[action.gameId]: action.info,
},
};
}
case "SET_LAUNCHED_GAME": {
return {
...state,
Expand Down
3 changes: 0 additions & 3 deletions kiosk/src/State/State.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import {
GameData,
BuiltSimJSInfo,
KioskState,
AllHighScores,
Notifications,
Expand All @@ -14,7 +13,6 @@ export type AppState = {
selectedGameId?: string;
launchedGameId?: string;
lockedGameId?: string;
builtGamesCache: { [gameId: string]: BuiltSimJSInfo };
allHighScores: AllHighScores;
kioskCode?: string;
kioskCodeExpiration?: number;
Expand All @@ -31,7 +29,6 @@ export const initialAppState: AppState = {
kioskState: KioskState.MainMenu,
allGames: [],
mostRecentScores: [],
builtGamesCache: {},
allHighScores: {},
notifications: [],
clean: false,
Expand Down
11 changes: 1 addition & 10 deletions kiosk/src/State/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { HighScore, BuiltSimJSInfo } from "../Types";
import { HighScore } from "../Types";
import { stateAndDispatch } from "./AppStateContext";

function getHighScores(gameId: string | undefined): HighScore[] {
Expand All @@ -22,18 +22,9 @@ function getSelectedGameId(): string | undefined {
return state.selectedGameId;
}

function getBuiltGame(gameId: string | undefined): BuiltSimJSInfo | undefined {
const { state } = stateAndDispatch();
if (!gameId) {
return undefined;
}
return state.builtGamesCache[gameId];
}

export {
stateAndDispatch,
getHighScores,
getSelectedGameIndex,
getSelectedGameId,
getBuiltGame,
};
8 changes: 0 additions & 8 deletions kiosk/src/Transforms/addBuiltGame.ts
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions kiosk/src/Types/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,6 @@ export type HighScoreWithId = HighScore & {
id: string;
};

export type BuiltSimJSInfo = {
js: string;
targetVersion?: string;
funArgs?: string[];
parts?: string[];
usedBuiltinParts?: string[];
};

export enum KioskState {
MainMenu = "MainMenu",
PlayingGame = "PlayingGame",
Expand Down
15 changes: 15 additions & 0 deletions kiosk/src/Utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,18 @@ export function nodeListToArray<U extends Node>(list: NodeListOf<U>): U[] {
}
return out;
}

// Copied from pxt.Utils, modified to skip undefined values.
export function stringifyQueryString(url: string, qs: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is more work and may not be needed, but I'm just wondering if there's benefit to defining a type for the object that is passed in for the query strings instead of just using any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied this from pxt.Util in order to fix what seemed like a bug. Given that we use this function in so many places, I felt it too risky to fix it in place without causing a regression somewhere. The testing surface is quite large. The bug is that when undefined is passed as a param value, it gets written to the url as key=undefined. The correct behavior, in my view, is to not include that param on the url at all.

I'll file an issue to track it.

for (let k of Object.keys(qs)) {
if (qs[k] != null) {
if (url.indexOf("?") >= 0) {
url += "&";
} else {
url += "?";
}
url += encodeURIComponent(k) + "=" + encodeURIComponent(qs[k]);
}
}
return url;
}
Loading