Conversation
| Msfs2024 = 'msfs2024', | ||
| } | ||
|
|
||
| export type TypeOfSimulator = `${Simulators}` | Simulators; |
There was a problem hiding this comment.
These two types look equivalent.
There was a problem hiding this comment.
The first type resolves to the raw strings of the enum while the second resolves to the enum. The acceptance of raw strings seems no longer required with the current fully typed implementation, so I have removed the union type.
There was a problem hiding this comment.
Update: It is still required to union these two types because the configuration is violating the type if it doesn't include the type '${Simulators}', as it is expected to verify against the raw string. Using a union of the types seems a better option than using a translation layer. Of course one could discard the second type and use raw strings within the installer program code too, however that seems less type safe.
While the fetched live configuration is not checked against at runtime, in my opinion it is better to have a standardized handling together with the shipped defaultconfiguration as well as using the same types for validation in the installer-data repo
(Idk why tslint didn't even complain about the defaultconfiguration, the last time I edited this...)
…ger required" This reverts commit 22e07e1.
flogross89
left a comment
flogross89
left a comment
There was a problem hiding this comment.
Thanks a lot! Overall approach looks valid and doable.
I don't have any experience with the installer codebase though, so take my approval with a grain of salt. Given how few potential reviewers there are though, I'd say we continue with merging and release testing.
|
@Benjozork Thanks a lot for the review! |
3 participants
Summary of Changes
Screenshots (if necessary)
Additional context
I initially intended addons to be able to support multiple simulators without needing two entries. This turned out to only be possible with a translation layer due to the need for individual addon keys for state handling, as well as the facilitation of chosing which set of paths to use. As in the long term, the configuration of an addon between the two MSFS simulators will diverge either way, I have opted for not creating this translation layer to ease the maintainability of the code.
Requires the use of an adapted configuration (flybywiresim/installer-data#56) or enabling force use local configuration.
Old versions of the installer (pre this PR) are unable to filter addons by simulator. Having a configuration with an MSFS 2020 and an MSFS 2024 version of an addon will therefore lead to having both versions shown and detected as installed. The new installer version will filter out any addons without the simulator property. A potential course of action to mitigate this issue is to initially release the new config alongside the new installer version without MSFS 2024 addons to allow for a transition.
Discord username (if different from GitHub):
foxtrotsierra