MagicMirrorOrg / MagicMirror

MagicMirror² is an open source modular smart mirror platform. With a growing list of installable modules, the MagicMirror² allows you to convert your hallway or bathroom mirror into your personal assistant.
http://magicmirror.builders
MIT License
19.76k stars 4.2k forks source link

Logic in config.js breaks EVERYTHING #1563

Closed jacob-ebey closed 5 years ago

jacob-ebey commented 5 years ago

Platform: ANY

Node Version: ANY

MagicMirror Version: v2.

Description: Adding ANY logic to config.js gives the error message as if a config does not exist. The "config:check" script still passes.

Steps to Reproduce: Add any logic/requires to the top of config.js. The following line breaks it:

var fs = require("fs");

Expected Results: I expect to be able to add logic to the config.js file to load settings off disk from a JSON file in the same directory.

Actual Results: The "Please create a config file" screen appears.

Configuration:

/* Magic Mirror Config Sample
 *
 * By Michael Teeuw http://michaelteeuw.nl
 * MIT Licensed.
 *
 * For more information how you can configurate this file
 * See https://github.com/MichMich/MagicMirror#configuration
 *
 */

var fs = require("fs");

var config = {
    address: "localhost", // Address to listen on, can be:
                          // - "localhost", "127.0.0.1", "::1" to listen on loopback interface
                          // - another specific IPv4/6 to listen on a specific interface
                          // - "", "0.0.0.0", "::" to listen on any interface
                          // Default, when address config is left out, is "localhost"
    port: 8080,
    ipWhitelist: ["127.0.0.1", "::ffff:127.0.0.1", "::1"], // Set [] to allow all IP addresses
                                                           // or add a specific IPv4 of 192.168.1.5 :
                                                           // ["127.0.0.1", "::ffff:127.0.0.1", "::1", "::ffff:192.168.1.5"],
                                                           // or IPv4 range of 192.168.3.0 --> 192.168.3.15 use CIDR format :
                                                           // ["127.0.0.1", "::ffff:127.0.0.1", "::1", "::ffff:192.168.3.0/28"],

    language: "en",
    timeFormat: 24,
    units: "metric",

    modules: [
        {
            module: "alert",
        },
        {
            module: "updatenotification",
            position: "top_bar"
        },
        {
            module: "clock",
            position: "top_left"
        },
        {
            module: "calendar",
            header: "US Holidays",
            position: "top_left",
            config: {
                calendars: [
                    {
                        symbol: "calendar-check",
                        url: "webcal://www.calendarlabs.com/templates/ical/US-Holidays.ics"
                    }
                ]
            }
        },
        {
            module: "compliments",
            position: "lower_third"
        },
        {
            module: "currentweather",
            position: "top_right",
            config: {
                location: location.location,
                locationID: "",  //ID from http://bulk.openweathermap.org/sample/; unzip the gz file and find your city
                appid: "YOUR_OPENWEATHER_API_KEY"
            }
        },
        {
            module: "weatherforecast",
            position: "top_right",
            header: "Weather Forecast",
            config: {
                location: location.location,
                locationID: "5128581",  //ID from https://openweathermap.org/city
                appid: "YOUR_OPENWEATHER_API_KEY"
            }
        },
        {
            module: "newsfeed",
            position: "bottom_bar",
            config: {
                feeds: [
                    {
                        title: "New York Times",
                        url: "http://www.nytimes.com/services/xml/rss/nyt/HomePage.xml"
                    }
                ],
                showSourceTitle: true,
                showPublishDate: true
            }
        },
    ]

};

/*************** DO NOT EDIT THE LINE BELOW ***************/
if (typeof module !== "undefined") {module.exports = config;}

Additional Notes: This is a VERY straight forward reproduction case that cripples the configuration capabilities beyond what anyone should consider acceptable. If we can't add logic to the config.js file, why is it not just a JSON file?

justjim1220 commented 5 years ago

If you are wanting to add logic such as fps, you need to create a node_helper.js file

On Sat, Feb 9, 2019, 04:42 Jacob Ebey <notifications@github.com wrote:

Platform: ANY

Node Version: ANY

MagicMirror Version: v2.

Description: Adding ANY logic to config.js gives the error message as if a config does not exist. The "config:check" script still passes.

Steps to Reproduce: Add any logic/requires to the top of config.js. The following line breaks it:

var fs = require("fs");

Expected Results: I expect to be able to add logic to the config.js file to load settings off disk from a JSON file in the same directory.

Actual Results: The "Please create a config file" screen appears.

Configuration:

/ Magic Mirror Config Sample By Michael Teeuw http://michaelteeuw.nl MIT Licensed. For more information how you can configurate this file See https://github.com/MichMich/MagicMirror#configuration */ var fs = require("fs"); var config = { address: "localhost", // Address to listen on, can be: // - "localhost", "127.0.0.1", "::1" to listen on loopback interface // - another specific IPv4/6 to listen on a specific interface // - "", "0.0.0.0", "::" to listen on any interface // Default, when address config is left out, is "localhost" port: 8080, ipWhitelist: ["127.0.0.1", "::ffff:127.0.0.1", "::1"], // Set [] to allow all IP addresses // or add a specific IPv4 of 192.168.1.5 : // ["127.0.0.1", "::ffff:127.0.0.1", "::1", "::ffff:192.168.1.5"], // or IPv4 range of 192.168.3.0 --> 192.168.3.15 use CIDR format : // ["127.0.0.1", "::ffff:127.0.0.1", "::1", "::ffff:192.168.3.0/28"],

language: "en", timeFormat: 24, units: "metric",

modules: [ { module: "alert", }, { module: "updatenotification", position: "top_bar" }, { module: "clock", position: "top_left" }, { module: "calendar", header: "US Holidays", position: "top_left", config: { calendars: [ { symbol: "calendar-check", url: "webcal://www.calendarlabs.com/templates/ical/US-Holidays.ics" } ] } }, { module: "compliments", position: "lower_third" }, { module: "currentweather", position: "top_right", config: { location: location.location, locationID: "", //ID from http://bulk.openweathermap.org/sample/; unzip the gz file and find your city appid: "YOUR_OPENWEATHER_API_KEY" } }, { module: "weatherforecast", position: "top_right", header: "Weather Forecast", config: { location: location.location, locationID: "5128581", //ID from https://openweathermap.org/city appid: "YOUR_OPENWEATHER_API_KEY" } }, { module: "newsfeed", position: "bottom_bar", config: { feeds: [ { title: "New York Times", url: "http://www.nytimes.com/services/xml/rss/nyt/HomePage.xml" } ], showSourceTitle: true, showPublishDate: true } }, ]

}; / DO NOT EDIT THE LINE BELOW /if (typeof module !== "undefined") {module.exports = config;}

Additional Notes: This is a VERY straight forward reproduction case that cripples the configuration capabilities beyond what anyone should consider acceptable. If we can't add logic to the config.js file, why is it not just a JSON file?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/MichMich/MagicMirror/issues/1563, or mute the thread https://github.com/notifications/unsubscribe-auth/AaT_6RcOvVeJHLIXBHFVcTryIRm9zj4wks5vLqYngaJpZM4ayfAZ .

jacob-ebey commented 5 years ago

I literally just want to be able to call fs.readFileSync at the top of the config/config.js to load a JSON file that will be located at config/timezone.json, so if you have a recommendation here that isn't cryptic, I'd very much appreciate it.

justjim1220 commented 5 years ago

Not trying to be cryptic, apologies... But, the only things that can be added to the config.js file are modules. If you have a module that displays the timezone from a json file, then you need to call a node_helper.js file to communicate between the module.js and the json file so that it can be displayed on your mirror.

Question I have... what are you trying to accomplish? If you are just trying to make sure you are getting results from the correct timezone, that is already pretty much encoded in the main MM programming.

If you are trying to create a module that requires the timezone, it is done very easily with moment.js, which is also included with MM.

Maybe you could be more specific with the reason you are wanting to add fps to the config.js file?

justjim1220 commented 5 years ago

In my opinion, you are fairly correct in thinking that the config.js is a glorified json file. As it is the main communication between the modules and MM.

jacob-ebey commented 5 years ago

A colleague is going to be giving a few out as gifts, and the timezone they are going to end up in is not known to us.

The module configuration for the weatherforecast module for example, has a "location" config property that I'd like to be able to fill at startup based on an API call to http://ip-api.com/json

With requests in node being strictly async, as part of the OS startup procedure, there is a simple script that queries that URL and dumps the contents to "MagicMirror/config/location.json".

I'd like to be able to, in the config.js file, load that location.json contents and use the "city" and "regionName" values to set the "location" property of the "weatherforcast" module.

This is not creating a new module, or modifying the logic of an existing one, but rather an attempt at creating a zero config setup for when the device is moved between timezones.

jacob-ebey commented 5 years ago

I greatly appreciate your speedy responses BTW :)

justjim1220 commented 5 years ago

I think I understand what you are wanting, Let me look at a few things and get back with you in a few minutes...

jacob-ebey commented 5 years ago

If it helps, here is the contents of the config folder that I expect to work, but does not: config.zip

Everything else is 1:1 with the master branch.

justjim1220 commented 5 years ago

So, these are going to be going to locations other than Seattle?

jacob-ebey commented 5 years ago

Yes, and when the OS (linux) boots, before the MM server is spun up, the timezone.json file is fetched by an external application and dumped to the config folder.

justjim1220 commented 5 years ago

you do realize that the weather modules also require an API key from OpenWeather?

it will need to be put in by each individual user, their own registered API key So, if they are going to have to do that much, they may as well also be privy to putting in their own city codes.

i think it may be easier for you to write an external script that asks the user to input the info at the start up of MM.

I don't see any way possible, for what you are trying to accomplish, for it to work like you are thinking it will. Inputting the timezone alone will not get the users any output on MM from the weather modules.

jacob-ebey commented 5 years ago

They will "ship" with the API key in place. The idea is to send them to non-technical people as a gift, and when they plug them in, their timezone info is gathered and used.

I'll look through the codebase more tomorrow and see if I can find an acceptable workaround. Do you happen to know where the weather module source is located? It seems I may have to just update the module it's self, but that would mean updating 2 or more modules, instead of allowing a "passthrough" of a value off disk.

justjim1220 commented 5 years ago

The weather modules also give the option to use DarkSky, which is what I prefer. Again, it requires a user registered API from DarkSky for it to work. without any type of API, none of the weather modules will have any type of output.

jacob-ebey commented 5 years ago

The issue isn't that the weather modules aren't displaying, it's the issue of adding:

var fs = require("fs");

to the config.js file shows the "no config screen": image

justjim1220 commented 5 years ago

I don't think you can use the same API key for several different locations. They will have to have their own registered API key. If you plan on putting in the API key for them, then you could just as easily put in the rest of the location info as well.

jacob-ebey commented 5 years ago

Again, the locations are not known. Imagine trying to give this as a white elephant gift and an out of town sales person picks it who's entire computer knowledge consists of hitting power and opening outlook.

justjim1220 commented 5 years ago

right, because the code var fs = require("fs"); is not a valid entry into the config.js file. So, it is going to see it as an error

that code does not work on it's own inside the config.js It has to be encoded into a module, as in a node_helper.js file that communicates between the module and the json file to give the wanted output on the display.

justjim1220 commented 5 years ago

I am seeing that. But, I see no possible way for any of the weather modules to work the way you are wanting without some type of user input.

jacob-ebey commented 5 years ago

Ok, that's what I'm not understanding at this point in time. I was introduced to this issue/codebase just a few hours ago so I'll do some deeper digging before I continue to pester you.

Much appreciated is your speed and effort though :)

justjim1220 commented 5 years ago

I will do some further investigating. I will get back with you and let you know if I find a solution to your situation I feel that I will be letting you know there is no solution that you seek

You are not pestering me one bit! I am always glad to help out when I can.

justjim1220 commented 5 years ago

just a thought... if these are considered'white elephant' gifts, send me one! LMAO! would be the best ;white elephant gift I ever recieved!!! 😁

jacob-ebey commented 5 years ago

The roommate is assembling them, but if they turn out nice I may be doing a round of them myself :)

jacob-ebey commented 5 years ago

And it seems that if I can get the "socketNotificationReceived" method to work in the main script (not the node_helper) then I will easily be able to accomplish what I am looking to do. If it works out after I get some sleep and am not staring at a screen with glass eyes, expect a PR with a new config property of "autoLocation" for both the "currentweather" and "weatherforecast" modules :)

justjim1220 commented 5 years ago

I will look for it I am anxious to see if you can pull this off! I can only imagine the possibilities of MM with this kind of script added in Not just for the weather modules, but for many of the different modules that are created for MM!

jacob-ebey commented 5 years ago

Ok, the PR is out: #1564

jacob-ebey commented 5 years ago

I also added autoTimezone to the clock.

fewieden commented 5 years ago

It is not possible to use var fs = require('fs'); in the config js, because it gets evaluated in the browser and not the nodejs side. Browsers don't have the require method defined.

Why is it not a plain JSON file? Because you can use all native javscript features. As example you can have a look into the config of https://github.com/eouia/MMM-NotificationTrigger#configuration-sample which wouldn't be possible in JSON.

jacob-ebey commented 5 years ago

@fewieden thanks for the example. I already solved the issue I was facing and a PR is out for it.

MichMich commented 5 years ago

I merged you PR. But to add to the discussion: the reason it is javascript in stead of json, is that it allows modules to use callbacks as configuration options. The reason your example breaks is -as others say- because you insert a node.js only statement in a file that is used by both the server as well as the browser. An easy solution would have been something like this:

if (typeof module !== "undefined") {
    var fs = require('fs');
}
jacob-ebey commented 5 years ago

Understood at this point. I had originally (before actually looking though the project source) figured it was a server only or even a "build" time file. I was introduced to the project pretty late that night by the roommate who had brought up the fact there wasn't an easy way to do autolocation on the gifts he was going to be giving to people in unknown locations who would not have the skills to do the configuration themselves.

I might become a regular contributor here after the pleasant experience and quick help from you guys :)

As a fun little thing, I was thinking through other ways a project like this could be approached and whipped up my own little "clone" if you are interested in some "new" ideas for a v3 architecture: https://github.com/jacob-ebey/Mirror2

Granted it's not thought through as long as this project, I have some ideas for how to handle modules that I will be implementing there as a POC before I bring anything here.