MagicMirror
MagicMirror copied to clipboard
Fix crash possibility if `module: <name>` is not defined
Fix #3442
Just a thought: Wouldn't that be a thing that belongs in check_config.js?
yes, also.
check_config.js check only syntax but it does not check the contents of the file
should we make some other checks on it too ?
well, this one is the first actual content that is important.. altho position should be checked too.. at least valid if specified.. that is another crash..
the user that reporteds the split problem said they ran config:check but it was good..
So, we have to check module: <name>and position: <valid position> (if defined)
we check it on check_config.js and MM core ?
Personally, I think it's essential to apply the rules to both
I think so.. never crash..
ok, I will see to code it
Right, I will try to add needed rules with eslint in check_config.js ;)
I have never done this kind of test. bugsounet, it's time to learn :)
module name error:
position error:
No error:
I'm not sure that i can integrate it into eslint
What do you think @sdetweil ?
Before approuve some new deps. Thanks to review this :>
If I use a module with a valid position and disabled: true , e.g.
{
module: "weather",
header: "default weather module current",
position: "top_left",
disabled: true,
config: {
weatherProvider: "openmeteo",
lat: 50.17659,
lon: 8.62685,
useCorsProxy: true,
roundTemp: true,
showPrecipitationProbability: true,
showHumidity: "wind",
showUVIndex: true,
appendLocationNameToHeader: false,
windUnits: "kmh",
showFeelsLike: false,
bridgeId: "abc",
},
},
I get a warning
[2024-06-07 23:21:00.091] [WARN] No module name found for this configuration: {
module: 'weather',
header: 'default weather module current',
position: 'top_left',
disabled: true,
config: {
weatherProvider: 'openmeteo',
lat: 50.17659,
lon: 8.62685,
useCorsProxy: true,
roundTemp: true,
showPrecipitationProbability: true,
showHumidity: 'wind',
showUVIndex: true,
appendLocationNameToHeader: false,
windUnits: 'kmh',
showFeelsLike: false,
bridgeId: 'abc'
}
}
Is this intended? I think we need no such warning for disabled modules ...
i agree, no warning there.
sure, it's not logical
fixed
It's boring to redo the packages :/
ping @rejas
@rejas:
-
I see that postions is again used in main.js. So I use it with MM.getAvailableModulePositions --> I able to have all position in loader.js
-
I have move position utils in utils.js --> used in app.js with Utils.moduleHasValidPosition()
@rejas: tell me
- If it's ok for you or not ?
- If you want really push it for MM² v2.28 ?
Sorry to say this but it's discouraging (this PR is open since May 10) :/
@rejas: tell me
* If it's ok for you or not ? * If you want really push it for MM² v2.28 ?Sorry to say this but it's discouraging (this PR is open since May 10) :/
Of course its ok for me (I do trust your code :-) I was jsut pondering a lot if we could move ALL the position usages into the same file. but that would be as we say in germany: in schönheit sterben (meaning "to die in beauty" aka to work too much and never finish :-D
So sorry for my slow approbval, but will do now!