Conversation
guillaumearm
left a comment
There was a problem hiding this comment.
Here is a first static review of the code, I don't have tested yet.
Tomorrow I'll test it on my current fika modpack.
| const exfils = ALL_EXFILS[mapName] ?? []; | ||
| const customExfils = NEW_CUSTOM_EXFILS[mapName] ?? []; | ||
| exfils.push(...customExfils); |
There was a problem hiding this comment.
| const exfils = ALL_EXFILS[mapName] ?? []; | |
| const customExfils = NEW_CUSTOM_EXFILS[mapName] ?? []; | |
| exfils.push(...customExfils); | |
| const vanillaExfils = ALL_EXFILS[mapName] ?? []; | |
| const customExfils = NEW_CUSTOM_EXFILS[mapName] ?? []; | |
| const exfils = { | |
| ...vanillaExfils, | |
| ...customExfils, | |
| }; |
everytime isValidExfilForMap is called, this will mutate ALL_EXFILS and add several time the same custom exfils.
mutations is the devil.
src/custom-extracts.ts
Outdated
| public getJsonFiles(directory: string): string[] { | ||
| try { | ||
| return fs.readdirSync(path.resolve(__dirname, directory)) | ||
| .filter(file => file.endsWith(".json")); | ||
| } catch (err) { | ||
| console.error(`Error reading directory ${directory}:`, err); | ||
| return []; | ||
| } | ||
| } | ||
|
|
||
| public loadConfigs(files: string[], directory: string): CustomExitConfig[] { | ||
| const configs: CustomExitConfig[] = []; | ||
|
|
||
| for (const file of files) { | ||
| try { | ||
| const filePath = path.resolve(__dirname, directory, file); | ||
| const rawData = fs.readFileSync(filePath, "utf8"); | ||
| configs.push(...JSON.parse(rawData)); | ||
| } catch (err) { | ||
| console.error(`Error reading file ${file}:`, err); | ||
| } | ||
| } | ||
|
|
||
| return configs; | ||
| } |
There was a problem hiding this comment.
I think reading json files should be done only once outside of this class. (the readed custom extracts can be just passed into the CustomExtracts constructor)
| if (!this.config.enabled) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
This would break the current uninstall procedure.
This check is already done few lines later.
|
|
||
| public postSptLoad(container: DependencyContainer): void { | ||
| this.container = container; | ||
| this.instanceManager.postDBLoad(container); |
There was a problem hiding this comment.
Is this line should be moved into a real postDBLoad ? because we are in the postSptLoad here.
There was a problem hiding this comment.
TODO: add a new configs/ExampleCustomExtracts/Tooltips.json
There was a problem hiding this comment.
TODO: add a new configs/ExampleCustomExtracts/config.json
There was a problem hiding this comment.
TODO: add a new configs/ExampleCustomExtracts/customExtracts/exfilConfig.json
There was a problem hiding this comment.
TODO: add a new configs/ExampleCustomExtracts/player_spawnpoints.json
| ], | ||
| }; | ||
|
|
||
| const ALL_NEW_CUSTOM_EXFILS = { |
There was a problem hiding this comment.
Maybe we can introduce a new config field enable_custom_exfils that will bypass the validation of the exfils name.
For a first iteration it seems enough.
|
Don't worry @GrooveypenguinX I won't forget this PR. Now I'm more comfortable with C# and client-side modding. I think I'll make a standalone mod based on your implementation. stay tuned |
Server:
Client: