-
Notifications
You must be signed in to change notification settings - Fork 69
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
feat(config-json): Only either bundle or load from remote #291
Changes from all commits
c4d8b5a
ff264be
3bb6532
d6e5450
976c4de
d428775
9735b17
d0f937f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ RUN npm i -g [email protected] | |
# Build arguments | ||
ARG DOWNLOAD_SOUNDS=false | ||
ARG DISABLE_SERVICE_WORKER=false | ||
ARG CONFIG_JSON_SOURCE=REMOTE | ||
# TODO need flat --no-root-optional | ||
RUN node ./scripts/dockerPrepare.mjs | ||
RUN pnpm i | ||
|
@@ -22,8 +23,8 @@ RUN if [ "$DOWNLOAD_SOUNDS" = "true" ] ; then node scripts/downloadSoundsMap.mjs | |
# ENTRYPOINT ["pnpm", "run", "run-all"] | ||
|
||
# only for prod | ||
RUN GITHUB_REPOSITORY=zardoy/minecraft-web-client \ | ||
DISABLE_SERVICE_WORKER=$DISABLE_SERVICE_WORKER \ | ||
RUN DISABLE_SERVICE_WORKER=$DISABLE_SERVICE_WORKER \ | ||
CONFIG_JSON_SOURCE=$CONFIG_JSON_SOURCE \ | ||
pnpm run build | ||
|
||
# ---- Run Stage ---- | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
import { disabledSettings, options, qsOptions } from './optionsStorage' | ||
import { miscUiState } from './globalState' | ||
import { setLoadingScreenStatus } from './appStatus' | ||
|
||
export type AppConfig = { | ||
// defaultHost?: string | ||
// defaultHostSave?: string | ||
defaultProxy?: string | ||
// defaultProxySave?: string | ||
// defaultVersion?: string | ||
peerJsServer?: string | ||
peerJsServerFallback?: string | ||
promoteServers?: Array<{ ip, description, version? }> | ||
mapsProvider?: string | ||
|
||
appParams?: Record<string, any> // query string params | ||
|
||
defaultSettings?: Record<string, any> | ||
forceSettings?: Record<string, boolean> | ||
// hideSettings?: Record<string, boolean> | ||
allowAutoConnect?: boolean | ||
pauseLinks?: Array<Array<Record<string, any>>> | ||
} | ||
|
||
export const loadAppConfig = (appConfig: AppConfig) => { | ||
if (miscUiState.appConfig) { | ||
Object.assign(miscUiState.appConfig, appConfig) | ||
} else { | ||
miscUiState.appConfig = appConfig | ||
} | ||
|
||
if (appConfig.forceSettings) { | ||
for (const [key, value] of Object.entries(appConfig.forceSettings)) { | ||
if (value) { | ||
disabledSettings.value.add(key) | ||
// since the setting is forced, we need to set it to that value | ||
if (appConfig.defaultSettings?.[key] && !qsOptions[key]) { | ||
options[key] = appConfig.defaultSettings[key] | ||
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. Will break setting from qs params , eed to fix |
||
} | ||
} else { | ||
disabledSettings.value.delete(key) | ||
} | ||
} | ||
} | ||
} | ||
|
||
export const isBundledConfigUsed = !!process.env.INLINED_APP_CONFIG | ||
|
||
if (isBundledConfigUsed) { | ||
loadAppConfig(process.env.INLINED_APP_CONFIG as AppConfig ?? {}) | ||
} else { | ||
void window.fetch('config.json').then(async res => res.json()).then(c => c, (error) => { | ||
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 probably use 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. Hm, I was avoiding any usage of top level awaits because I think it can break the app in some unexpected way and don't really see any benefit of using it 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. Well I don't really see what is stopping the client from using the bundled config.json values until the remote one is loaded and imo await is the proper way to force async queries to block the app until it's loaded? If it really breaks something then that would be a bug elsewhere (and if it fails to load the file then it should of course break as the app doesn't work without the config) |
||
// console.warn('Failed to load optional app config.json', error) | ||
// return {} | ||
setLoadingScreenStatus('Failed to load app config.json', true) | ||
}).then((config: AppConfig) => { | ||
loadAppConfig(config) | ||
}) | ||
} |
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.
🛠️ Refactor suggestion
Use native git commands instead of parsing config file.
The function attempts to extract the repository information by parsing the git config file directly with regex, which is fragile and may break with different git configurations or formats.
I recommend using git commands directly through
execSync
, which would be more robust:📝 Committable suggestion