-
Notifications
You must be signed in to change notification settings - Fork 151
Error loading main module tries to build temp file, fails, reports wrong error #331
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
Comments
Additional thoughts on ways to address this: Config option for something like Unfortunately, I don't even know what Remote-Control is trying to do when it "loads" a module, so I'm not a ton of help. |
As far as I understand it, it's about getting the default options and values of the modules. This is somehow different for the bundled modules than for the normal ones. I don't know if I'll be able to take a closer look soon. If someone else can do that, I would appreciate it. |
Just to be clear, “bundled” means the default apps that come with vanilla MM? Wouldn’t it be easier and more precise to identify theses by filepath instead of “if error, catch”? |
No, with "bundled" I mean modules like MMM-RAIN-MAP which are developed in TypeScript and have a build process to create the final file(s). Checkout the MMM-RAIN-MAP.js - it doesn't look like a normal module js file.
Identifying is not the problem. The problem is to load the defaults from those files. |
Can you please check if this issue also occurs if you don't use pm2? I can't reproduce it 🤔 |
Oh, so the catch-then-create-temp.js is actually designed for modules like this. I'll have to look at it more closely and see if I can come up with a solution.
I'd be happy to, but I don't actually know what the alternative method is. |
I mean, changing line 334 to: const newContent = `const Log = console;const document = navigator = window = {};document.createElement = function() { return {}; };\n${fs.readFileSync(filename, "utf8").replace('(Log)','')}`; Stops throwing errors. It just removes the weird "(Log)" from the end of the file. I couldn't tell you if the file is properly loaded as I do not use Remote-Control for anything having to do with RAIN-MAP. And yes, I confirmed that's the only instance of "(Log)" in the file. But that's obviously going to be hit or miss for other files of this type. A slightly safer though more verbose way of doing it would be something like:
Sometime someone is going to have to explain to me why some javascript files have a semicolon at the end of every line and others don't. |
I'm on vacation for the next few days 😎 I'll take a look at it afterward. |
Thanks for the PR! Unfortunately, I cannot recreate the problem. Not even with pm2. Since the manipulation of the bundled modules is already a bit hacky, I would like to understand the problem better before I merge the PR. Can you please test with the following minimal let config = {
address: "0.0.0.0",
port: 8080,
basePath: "/",
ipWhitelist: [],
logLevel: ["INFO", "LOG", "WARN", "ERROR", "DEBUG"],
modules: [
{
module: "MMM-RAIN-MAP",
position: "top_left"
},
{
module: "MMM-Remote-Control",
position: "bottom_left"
}
]
};
/** ************* DO NOT EDIT THE LINE BELOW ***************/
if (typeof module !== "undefined") { module.exports = config; }
Run that in the command line: cd ~/MagicMirror
npm run start
Most developers use semicolons at the end of every line in JavaScript for clarity and to avoid issues with Automatic Semicolon Insertion (ASI), while others skip them, trusting ASI to handle it. I would say that it's a matter of coding style preference. |
Seen. Will do when I have the opportunity and will report back. |
I just took a closer look at it, loading the defaults from bundled modules does not work (nor with or without your PR). We have to come up with something else. |
But at least the error message is gone! ;-) |
Ok, I think I have another solution conceptually, but I don't know if it applies broadly to all "bundled" modules. Most modules take the basic form of: Module.register("MMM-ModuleName", {
//<all the code>
}); MMM-RAIN-MAP, on the other hand, has the ! function(t) {
//<several functions and variables defined>
Module.register("MMM-RAIN-MAP", {
//<all the code>
});
}(Log); If you delete all of the framing stuff to get it back down to just the I've truly just taught myself enough javascript to muddle through patching modules, so this is not my area of expertise. One thing that would work here is something like: fileContent = 'Module.register' + fileContent.split('Module.register')[1].split(`}${fileContent.split('}')[fileContent.split('}').length - 1]}`)[0] (translated: look for the phrase This would come right before But i don't know if that would be AT ALL generalizable to other "bundled" modules. |
A new commit incorporating this has been pushed in #333. |
A creative approach, but unfortunately the defaults are not loaded. You can see that if you use a minimal config like above and open the settings for MMM-RAIN-Map here: http://localhost:8080/remote.html#settings-menu |
Interesting. I was testing by adding some logging in node_helper and finding that the defaults do make it to that stage. Didn't know there was such a page. I'll do more testing. |
Should be fixed with version 3.1.8 🙂 Please check. The default settings for the bundled modules are now retrieved from the frontend. This is not yet perfect, but better than before. |
I'm not getting an error anymore, but I did get this Log message:
|
As release 3.1.8 has solved/changed the situation, I am closing this issue. Thanks again for your helpful input! 🙂 I opened issue #335 for a related issue. |
OS
Debian 10
NodeJS Version
22
MagicMirror² Version
2.31
Remote Control Version
2.5.3
Did you try using just Remote Control alone with MM?
Description
MMM-Remote-Control tries to load /MMM-RAIN-MAP/MMM-RAIN-MAP.js, which fails initially, so it tries to build a temp.js file. Loading the reformatted temp.js also fails, which causes an error in the logs.
The error reported on the actual js file and the error reported on the temp.js are different, and only the latter gets reported in the log (which I do not believe is the real error).
temp.js is not deleted, so it just stays there.
Expected behavior
Current behavior
With module MMM-RAIN-MAP, MMM-Remote-Control tries to load the main js file, fails in
node_helper.js
at line 328.The apparent error is with the very end of MMM-RAIN-MAP.js where there is an additional
(Log)
after the usual final}
. I could not tell you why it's there, what it does, or why MMM-Remote-Control does not like it (the MMM-RAIN-MAP module works fine on its own).The above logging had to be added.
The catch block for line 328 says to build a temp.js file to hopefully correct some issues that apparently come from reading bundled apps. That temp.js file is created, but it's no use. Adding some logging shows a DIFFERENT error on the temp file than on the original file:
Again, the above logging had to be added for debug purposes.
After failing on the temp.js, vanilla Remote-Control reports the following error, which is both incorrect and not particularly helpful
(I confirmed this to be MMM-RAIN-MAP by some added logging, #330 .)
The temp.js file is never deleted.
So Remote-Control catches an error that (maybe?) it shouldn't catch.
Tries to build a temp.js file to get around an error that is not present.
Reports the wrong error because it reports the error on the temp.js.
And then doesn't delete the temp.js it creates.
Possible solution
Either be more lenient on the ending
(Log)
in the MMM-RAIN-MAP file, or more accurately report that error? (I tried to delete it from MMM-RAIN-MAP, but it breaks that module).More carefully select which files get written to temp files?
Clean up temp files after attempt?
Steps to reproduce
Install MMM-RAIN-MAP
Install MMM-Remote-Control
Check logs
Log
config.js
Additional info
No response
The text was updated successfully, but these errors were encountered: