Skip to content
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

add new env vars MM_MODULES_DIR and MM_CUSTOMCSS_FILE … #3530

Merged
merged 4 commits into from
Sep 18, 2024

Conversation

khassel
Copy link
Collaborator

@khassel khassel commented Sep 1, 2024

… for setting these things from outside (and overriding corresponding config.js properties config.foreignModulesDir and customCss)

  • remove elements from index.html when loading script or stylesheet files fails
  • removed config.paths.vendor (could never work because vendor is hardcoded in index.html) and renamed config.paths.modules to config.foreignModulesDir. The config.paths. ... properties were implemented in the initial commit in js/defaults.js but were never functional.
  • fixes app.js which didn't respect config.paths.modules before
  • as modules/defaults is directly set in many places in the source code restrict config.paths.modules to foreign modules (it has never worked for default modules), now renamed to config.foreignModulesDir
  • adds new /env section in server.js for getting the new env vars in the browser
  • fixes TODO in server.js so test directories are now only published when running tests

These changes allow changing some main paths from outside mm with the new env vars. You now can put all user stuff into one directory, e.g. the config dir:

  • config.js as before
  • custom.css
  • foreign modules

This would simplify other setups e.g. the docker setup. At the moment we have to deal with 3 directories where 2 of them (modules and css) contains mixed stuff, which means mm owned files and user files. This can now simplified and leads to cleaner setups (if wanted).

@khassel khassel requested a review from rejas September 1, 2024 20:32
@sdetweil
Copy link
Collaborator

sdetweil commented Sep 1, 2024

hm. this will break modules which depend on their location being two directories up from the root and others that depend on folder names.
like those that need electron-rebuild in the root node_modules folder. and my mmm-config

will also break my upgrade script

what will be the effect on git pull if the deployed structure is different?

@khassel
Copy link
Collaborator Author

khassel commented Sep 1, 2024

the idea would be

  • to have modules/default/... (no change)
  • to have config/MMM-XY instead of modules/MMM-XY
  • to have config/custom.css instead of css/custom.css (which is already now possible by setting in config.js)

So can you explain what exactly would break?

what will be the effect on git pull if the deployed structure is different?

# Ignore users config file but keep the sample.
/config/*
!/config/config.js.sample

no effect because above is in .gitignore

@khassel
Copy link
Collaborator Author

khassel commented Sep 1, 2024

you can already now change the modules dir and custom.css location inside config.js, this PR only introduces additional env vars for setting this from outside.

@sdetweil
Copy link
Collaborator

sdetweil commented Sep 5, 2024

can either/both of these be outside the MagicMirror folder tree?

@khassel
Copy link
Collaborator Author

khassel commented Sep 6, 2024

can either/both of these be outside the MagicMirror folder tree?

I did not test this so far, will do and report.

I think at least modules folder must reachable from the browser over http://<server>:<port>/<modules-dir> so the choice of the folder is limited.

@khassel
Copy link
Collaborator Author

khassel commented Sep 6, 2024

both variables (or corresponding values in config.js) are restricted to the published directories in js/server.js:

			let directories = ["/config", "/css", "/fonts", "/modules", "/vendor", "/translations", "/env"];

@sdetweil
Copy link
Collaborator

sdetweil commented Sep 6, 2024

ok, so one could set modules to config, but not new dir foo, same for config to modules, but not foo

and does it require leading slash?

@khassel
Copy link
Collaborator Author

khassel commented Sep 6, 2024

ok, so one could set modules to config, but not new dir foo, same for config to modules, but not foo

yes.

and does it require leading slash?

no.

You can define it like

node@81ac3b4687ab:/opt/magic_mirror$ export MM_CUSTOMCSS_FILE=config/custom.css
node@81ac3b4687ab:/opt/magic_mirror$ export MM_CUSTOMCSS_FILE=/config/custom.css
node@81ac3b4687ab:/opt/magic_mirror$ export MM_CUSTOMCSS_FILE=/opt/magic_mirror/config/custom.css

All 3 variants works, the mm root dir is stripped in the code.

@sdetweil
Copy link
Collaborator

sdetweil commented Sep 6, 2024

ok, can cssfile be modules/foo.css

i misread cssfile, thinking config file and folder could be moved

@sdetweil
Copy link
Collaborator

sdetweil commented Sep 6, 2024

i always assumed cssfile was name of file, not location

@khassel
Copy link
Collaborator Author

khassel commented Sep 6, 2024

The default config.js key customCss is "css/custom.css". You can overwrite it in config.js or with the new env var MM_CUSTOMCSS_FILE. modules/foo.css works.

@sdetweil
Copy link
Collaborator

sdetweil commented Sep 6, 2024

thanks, i never thought one could change the css directory, only the filename

@sdetweil
Copy link
Collaborator

sdetweil commented Sep 7, 2024

I haven't checked the code yet, in all cases at the time a module is loaded are the config variables set so I can use those as the true path? vs examining for the env myself..

@sdetweil
Copy link
Collaborator

sdetweil commented Sep 7, 2024

in MMM-Config I launch a shell script to collect all the module defaults and generate the form object..
I have a bug now, in that I don't check for where the config.js is.. I 'assume' config/config.js and add on modules in modules
easy fix if the env variables are replicated into the config object

@khassel
Copy link
Collaborator Author

khassel commented Sep 7, 2024

at the moment this is still unmerged ...

I haven't checked the code yet, in all cases at the time a module is loaded are the config variables set so I can use those as the true path? vs examining for the env myself..

if the env variables are replicated into the config object

The new variables don't touch config.js. I don't know where you need the new variables, server or browser?
Depending on this

  • for server you can use getEnvVarsAsObj from server_functions.js
  • for browser you can do something like I implemented in loader.js
		const res = await fetch(`${location.protocol}//${location.host}/env`);
		return JSON.parse(await res.text());

I don't check for where the config.js is.. I 'assume' config/config.js

this is unrelated to this PR but you should check MM_CONFIG_FILE

@sdetweil
Copy link
Collaborator

sdetweil commented Sep 7, 2024

the config object after parsing is available to the browser sideof the module, so I could see the config.modules.path I was just wondering if you used the env to update that (as your PR comments say 'override' )

I don't really need the config object, and I don't care about css really.
the environment variables get passed to the node_helper in the process.env
so I can see these vars..

@khassel
Copy link
Collaborator Author

khassel commented Sep 7, 2024

the config object after parsing is available to the browser sideof the module, so I could see the config.modules.path I was just wondering if you used the env to update that (as your PR comments say 'override' )

thanks, didn't thought about this, but I think it would be the better way to change the values in the config object. Will check this and change it if it works.

@khassel khassel marked this pull request as draft September 7, 2024 21:39
@khassel
Copy link
Collaborator Author

khassel commented Sep 8, 2024

the config object after parsing is available to the browser sideof the module

the config object is available to the browser because of these lines in index.html:

    <script type="text/javascript" src="js/defaults.js"></script>
    <script type="text/javascript" src="#CONFIG_FILE#"></script>

(the #CONFIG_FILE# is replaced with real config.js file/path while loading)

So I stay with the current approach because otherwise I had to manipulate content of config.js or do other ugly things.

@khassel khassel marked this pull request as ready for review September 8, 2024 13:13
@sdetweil
Copy link
Collaborator

sdetweil commented Sep 8, 2024

understood. i forgot they are loaded from file

@khassel
Copy link
Collaborator Author

khassel commented Sep 10, 2024

@rejas from my side this is ready for review

@sdetweil
Copy link
Collaborator

did you change anything in the prior support config.paths.modules?

I moved a module to the config folder
added the paths: { modules: "config" }
to config.js
and on start got this
[2024-09-10 14:15:31.983] [WARN] No /home/sam/MagicMirror.old/js/../modules/MMM-Config/MMM-Config.js found for module: MMM-Config.
true, not there..
BUT the module was loaded to web page.. at config/MMM-Config/MMM-Config.js

Load script: config/MMM-Config/MMM-Config.js
module.js:489 Module registered: MMM-Config
MMM-Config.js:31 undefined is in init!   but not set up correctly 
       init: function () {
            Log.log(this.name + " is in init!");
       }

loader.js:133 Bootstrapping module: MMM-Config
loader.js:137 Scripts loaded for: MMM-Config
loader.js:175 Load stylesheet: config/MMM-Config/MMM-Config.css
loader.js:140 Styles loaded for: MMM-Config
translator.js:99 MMM-Config - Load translation: translations/en.json
loader.js:143 Translations loaded for: MMM-Config
MMM-Config.js:35 MMM-Config is starting!
       start: function () {
             Log.log(this.name + " is starting!");   // but ok here

BUT the node helper wasn't loaded.. oops..

this is 2.29-develop

but it loaded modules in the modules folder
Screenshot at 2024-09-10 14-46-33
note that the defaults also were directed there (config)

@khassel
Copy link
Collaborator Author

khassel commented Sep 10, 2024

did you change anything in the prior support config.paths.modules?

no

but there was a bug in the code so this couldn't work (because modules was hardcoded).

So what are you testing?

  • current develop branch
  • or content of this branch?

@sdetweil
Copy link
Collaborator

testing 2.29 prior to this PR..

but there was a bug in the code so this couldn't work (because modules was hardcoded).

so, YES, you had to change something to get this PR to work . the original code was broken

trying to test setting modules location thru config.js, before env variable..
(testing my MMM-Config module which needs to know where the modules are..and the config.js name)

@khassel
Copy link
Collaborator Author

khassel commented Sep 10, 2024

so, YES, you had to change something to get this PR to work . the original code was broken

line 162 in js/app.js

		let moduleFolder = `${__dirname}/../modules/${module}`;

was not using config.paths.modules but hardcoded modules

@sdetweil
Copy link
Collaborator

sdetweil commented Sep 11, 2024

ok, two things

  1. the paths object doesn't have the same override approach as the rest of config.js
    leave out the vendor: setting amd the url loaded says undefined/node_modules....

  2. even tho the modules js wasn't found, it gets injected anyhow..
    I moved a module from modules to config

    I added the paths { module:"config"} (see 1 above)
    I used the same config.js (should only load the one module moved to config)

    got errors (as expected)

2024-09-11 09:40:22.559] [WARN]  No /home/sam/MagicMirror.old/js/../config/MMM-WiFiPassword/MMM-WiFiPassword.js found for module: MMM-WiFiPassword. 
[2024-09-11 09:40:22.559] [LOG]   No helper found for module: MMM-WiFiPassword. 
[2024-09-11 09:40:22.559] [LOG]   No helper found for module: alert. 
[2024-09-11 09:40:22.559] [LOG]   No helper found for module: clock. 
[2024-09-11 09:40:22.586] [LOG]   Initializing new module helper ... 
[2024-09-11 09:40:22.586] [LOG]   Module helper loaded: calendar 
[2024-09-11 09:40:22.586] [WARN]  No /home/sam/MagicMirror.old/js/../config/MMM-MealieMenu/MMM-MealieMenu.js found for module: MMM-MealieMenu. 
[2024-09-11 09:40:22.586] [LOG]   No helper found for module: MMM-MealieMenu. 
[2024-09-11 09:40:22.586] [WARN]  No /home/sam/MagicMirror.old/js/../config/MMM-Ternary-clock/MMM-Ternary-clock.js found for module: MMM-Ternary-clock. 
[2024-09-11 09:40:22.586] [LOG]   No helper found for module: MMM-Ternary-clock. 
[2024-09-11 09:40:22.625] [LOG]   modules folder set at init to =modules 
[2024-09-11 09:40:22.634] [LOG]   Initializing new module helper ... 
[2024-09-11 09:40:22.634] [LOG]   Module helper loaded: MMM-Config 
[2024-09-11 09:40:22.634] [LOG]   No helper found for module: compliments. 
[2024-09-11 09:40:22.634] [WARN]  No /home/sam/MagicMirror.old/js/../config/MMM-page-indicator/MMM-page-indicator.js found for module: MMM-page-indicator. 
[2024-09-11 09:40:22.634] [LOG]   No helper found for module: MMM-page-indicator. 
[2024-09-11 09:40:22.634] [WARN]  No /home/sam/MagicMirror.old/js/../config/MMM-DateCounter/MMM-DateCounter.js found for module: MMM-DateCounter. 
[2024-09-11 09:40:22.634] [LOG]   No helper found for module: MMM-DateCounter. 
[2024-09-11 09:40:22.634] [WARN]  No /home/sam/MagicMirror.old/js/../config/MMM-KeyBindings/MMM-KeyBindings.js found for module: MMM-KeyBindings. 
[2024-09-11 09:40:22.634] [LOG]   No helper found for module: MMM-KeyBindings. 
[2024-09-11 09:40:22.635] [WARN]  No /home/sam/MagicMirror.old/js/../config/MMM-OpenWeatherMapForecast/MMM-OpenWeatherMapForecast.js found for module: MMM-OpenWeatherMapForecast. 

but the url was still added , like 'load' was successful ..
Screenshot at 2024-09-11 09-45-35

@khassel
Copy link
Collaborator Author

khassel commented Sep 11, 2024

well, I don't know exactly what you tested, AFAIS you are not testing this PR (the new env vars) but the config.js functionality (which was already there before this PR).

the paths object doesn't have the same override approach as the rest of config.js
leave out the vendor: setting amd the url loaded says undefined/node_modules....

yes, had this problem too, you must provide a full object under paths, otherwise the not provided keys are missing. Maybe objects are copied only on first level. AFAIS the property config.paths.vendor is useless (will not work with a value <> vendor) because we have vendor hardcoded in index.html. So we could "fix" this by removing config.paths.vendor and replacing config.paths.modules with e.g. config.modulesPath.

Let me know if I should update this PR with such a fix.

even tho the modules js wasn't found, it gets injected anyhow..

sorry, don't understand/don't know what you expect from me here

@sdetweil
Copy link
Collaborator

I tested this pr as a separate branch..

moved one module to the config folder from modules
add the paths:{ modules:"config"} to the config.js

then started MM

and looked in the browser development console, elements tab and see this
Screenshot at 2024-09-11 13-17-52

notice the 3 undefined/node_modules
for things in vendor

you confirmed its not handle the same way (Object .assign()) like the other config.js parms..
and I don't seen any other vendor/ hard coded in config.js
so it would have worked wiith the object.assign

second, on startup, the config.js references modules NOT found in the configured 'modules' folder,
AND correctly reports that the modulename.js cannot be found during startup (the messages above)

BUT it still considers them 'loaded' as their script link is still added to the in browser index.html..
that should not be..

I understand you are 'only' proposing env vars, BUT.. the previous (undocumented) built in support
didn't work, so you have had to fix it.. now you 'own' it for this PR. else this PR would not work either

using the env var MM_MODULES_DIR=config
(config.js paths commented out) produces the same results for issue 2.

Screenshot at 2024-09-11 13-28-10

I'm ok with not having vendor movable.. and having only the config.modulesPath

@sdetweil
Copy link
Collaborator

altho.. I am sure that on all file systems we run on, there is a link capacbility for those that want to move things around physically without us having to provide config options..

@khassel
Copy link
Collaborator Author

khassel commented Sep 11, 2024

@sdetweil fixed the things you mentioned above.

Tests are failing at the moment unrelated to this PR (did the tests on develop with same result).

@sdetweil
Copy link
Collaborator

crazy that we get these random failures...

@khassel
Copy link
Collaborator Author

khassel commented Sep 11, 2024

altho.. I am sure that on all file systems we run on, there is a link capacbility for those that want to move things around physically without us having to provide config options..

the basic idea is to use one dir (config) for all user stuff so I have only to mount one volume for the docker setup. I could use ln in the container to get things in one dir but there would still be the need to mount 3 volumes. Other point could be (not officially supported) windows where are no symlinks out of the box AFAIK.

@sdetweil
Copy link
Collaborator

windows 11 has symlinks
https://www.howtogeek.com/16226/complete-guide-to-symbolic-links-symlinks-on-windows-or-linux/

@sdetweil
Copy link
Collaborator

looks good both env and config parm

@khassel
Copy link
Collaborator Author

khassel commented Sep 18, 2024

I'm not willing to solve merge conflicts in the changelog over weeks/months, so can I have feedback from @rejas / @sdetweil if this can be merged?

@sdetweil
Copy link
Collaborator

I have tested it extensively, and I think it works well.. so yes, you can merge

@rejas rejas merged commit 659e0c7 into MagicMirrorOrg:develop Sep 18, 2024
6 checks passed
@khassel khassel deleted the file-struct branch September 18, 2024 18:27
rejas pushed a commit that referenced this pull request Sep 19, 2024
uses newsfeed test after copying this module to config dir

addition for #3530
@khassel khassel mentioned this pull request Sep 30, 2024
khassel added a commit that referenced this pull request Sep 30, 2024
## [2.29.0] - 2024-10-01

Thanks to: @bugsounet, @dkallen78, @jargordon, @khassel,
@KristjanESPERANTO, @MarcLandis, @rejas, @ryan-d-williams, @sdetweil,
@skpanagiotis.

> ⚠️ This release needs nodejs version `v20` or `v22`, minimum version
is `v20.9.0`

### Added

- [compliments] Added support for cron type date/time format entries mm
hh DD MM dow (minutes/hours/days/months and day of week) see
https://crontab.cronhub.io for construction (#3481)
- [core] Check config at every start of MagicMirror² (#3450)
- [core] Add spelling check (cspell): `npm run test:spelling` and handle
spelling issues (#3544)
- [core] removed `config.paths.vendor` (could not work because `vendor`
is hardcoded in `index.html`), renamed `config.paths.modules` to
`config.foreignModulesDir`, added variable `MM_CUSTOMCSS_FILE` which -
if set - overrides `config.customCss`, added variable `MM_MODULES_DIR`
which - if set - overrides `config.foreignModulesDir`, added test for
`MM_MODULES_DIR` (#3530)
- [core] elements are now removed from `index.html` when loading script
or stylesheet files fails
- [core] Added `MODULE_DOM_UPDATED` notification each time the DOM is
re-rendered via `updateDom` (#3534)
- [tests] added minimal needed node version to tests (currently v20.9.0)
to avoid releases with wrong node version info
- [tests] Added `node-libgpiod` library to electron-rebuild tests
(#3563)

### Removed

- [core] removed installer only files (#3492)
- [core] removed raspberry object from systeminformation (#3505)
- [linter] removed `eslint-plugin-import`, because it doesn't support
ESLint v9. We will reenter it later when it does.
- [tests] removed `onoff` library from electron-rebuild tests (#3563)

### Updated

- [weather] Updated `apiVersion` default from 2.5 to 3.0 (#3424)
- [core] Updated dependencies including stylistic-eslint
- [core] nail down `node-ical` version to `0.18.0` with exception
`allow-ghsas: GHSA-8hc4-vh64-cxmj` in `dep-review.yaml` (which should
removed after next `node-ical` update)
- [core] Updated SocketIO catch all to new API
- [core] Allow custom modules positions by scanning index.html for the
defined regions, instead of hard coded (PR #3518 fixes issue #3504)
- [core] Detail optimizations in `config_check.js`
- [core] Updated minimal needed node version in `package.json`
(currently v20.9.0) (#3559) and except for v21 (no security updates)
(#3561)
- [linter] Switch to ESLint v9 and flat config and replace
`eslint-plugin-unicorn` by `@eslint/js`
- [core] fix discovering module positions twice after #3450

### Fixed

- Fixed `checks` badge in README.md
- [weather] Fixed issue with the UK Met Office provider following a
change in their API paths and header info.
- [core] add check for node_helper loading for multiple instances of
same module (#3502)
- [weather] Fixed issue for respecting unit config on broadcasted
notifications
- [tests] Fixes calendar test by moving it from e2e to electron with
fixed date (#3532)
- [calendar] fixed sliceMultiDayEvents getting wrong count and
displaying incorrect entries, Europe/Berlin (#3542)
- [tests] ignore `js/positions.js` when linting (this file is created at
runtime)
- [calendar] fixed sliceMultiDayEvents showing previous day without
config enabled

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Michael Teeuw <michael@xonaymedia.nl>
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ross Younger <crazyscot@gmail.com>
Co-authored-by: Veeck <github@veeck.de>
Co-authored-by: Bugsounet - Cédric <github@bugsounet.fr>
Co-authored-by: jkriegshauser <joshuakr@nvidia.com>
Co-authored-by: illimarkangur <116028111+illimarkangur@users.noreply.github.com>
Co-authored-by: sam detweiler <sdetweil@gmail.com>
Co-authored-by: vppencilsharpener <tim.pray@gmail.com>
Co-authored-by: veeck <michael.veeck@nebenan.de>
Co-authored-by: Paranoid93 <6515818+Paranoid93@users.noreply.github.com>
Co-authored-by: Brian O'Connor <btoconnor@users.noreply.github.com>
Co-authored-by: WallysWellies <59727507+WallysWellies@users.noreply.github.com>
Co-authored-by: Jason Stieber <jrstieber@gmail.com>
Co-authored-by: jargordon <50050429+jargordon@users.noreply.github.com>
Co-authored-by: Daniel <32464403+dkallen78@users.noreply.github.com>
Co-authored-by: Ryan Williams <65094007+ryan-d-williams@users.noreply.github.com>
Co-authored-by: Panagiotis Skias <panagiotis.skias@gmail.com>
Co-authored-by: Marc Landis <dirk.rettschlag@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants