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

regular expression used as defaults value of a module, is corrupted. #3237

Closed
eouia opened this issue Oct 18, 2023 · 12 comments
Closed

regular expression used as defaults value of a module, is corrupted. #3237

eouia opened this issue Oct 18, 2023 · 12 comments

Comments

@eouia
Copy link
Contributor

eouia commented Oct 18, 2023

Recently, I found that when the regular expression is used as the default config value of a module, it would be corrupted.
I think it comes from configMerge or somewhere, but I cannot track exactly where the issue lies.

How to represent/prove

set some fields in defaults of MODULENAME.JS and config of config.js

// In module file
Module.register("TEST", {
  defaults: {
    rex_default1: /ab+c/,
    rex_default2: [ 'ab+c', /ab+c/, new RegExp('ab+c') ],
    rex_config1: null,
    rex_config2: [],
...
// in config.js
{
  module: 'TEST',
  config: {
    rex_config1: /ab+c/,
    rex_config2: [ 'ab+c', /ab+c/, new RegExp('ab+c') ],
...
//

Let's check which values are there.

start: function () {
    console.log(this.config)
}

The Result;
image

@sdetweil
Copy link
Collaborator

more than likely the merge under the covers does stringify which DOES NOT handle code expressions by default.

I had to fix this same problem in my mmm-config module

@sdetweil
Copy link
Collaborator

well, the default is Object.assign() which only does shallow copy and only enumerable items.
Screenshot at 2023-10-18 08-41-55

@eouia
Copy link
Contributor Author

eouia commented Oct 18, 2023

It doesn't matter.
General Object.assign() itself do it's role well.
image
I think JSON.stringify() somewhere for deepMerging or whatever makes this issue.

@sdetweil
Copy link
Collaborator

sdetweil commented Oct 18, 2023

is that the same data as the new RegExp() which you reported in first message?

@eouia
Copy link
Contributor Author

eouia commented Oct 18, 2023

??? sorry, I cannot catch your question.

In my original post,
rex_config1 and rex_config2 were imported from 'config.js' and overrided as expected.
But,
rex_default1 and rex_default2 were corrupted. They should remain but changed as /(?:)/, and this value would definitely be derived from new RegExp(false|null|undefined). So I guess somewhere JSON.stringify fails to convert proper string, so it might have undefined or false then tried to revamp RegExp Object from it.

@sdetweil
Copy link
Collaborator

sdetweil commented Oct 18, 2023

config.js is parsed by JavaScript( in the browser by the script tag) and is an object, not a string, the new RegExp() is converted to an object, the output of the new . I believe this is actually a function object, which stringify doesn't handle be default
index.html

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

@eouia
Copy link
Contributor Author

eouia commented Oct 18, 2023

So, is this not a bug and designed as expected? A module cannot get a regular expression object as a default value.
If so, OK.

@sdetweil
Copy link
Collaborator

sdetweil commented Oct 18, 2023

i would try to set the deepMerge: true option and see if that makes a difference.. it doesn't use Object.assign()

I don't know if its a 'bug'.. there are no default modules that have code in their config settings..

but my pythonPrint does

transform:(data)=>{return data.replace(/\n/g,"<br>")}

and that gets processed correctly..

@eouia
Copy link
Contributor Author

eouia commented Oct 18, 2023

Yes, I can avoid this problem by assigning regexp objects in code (usually start()) manually when the user configuration is not overriding them. It is just a little annoying and unexpected behaviour. That's all.

@sdetweil
Copy link
Collaborator

the setting to add to your module before config is

 configDeepMerge: true,

@khassel khassel added the bug label Oct 18, 2023
@khassel
Copy link
Collaborator

khassel commented Oct 18, 2023

the problem is this line in js/module.js, before cloneObject the values are o.k., after the call they are corrupted.

the cloneObject function is defined in js/class.js here.

@khassel
Copy link
Collaborator

khassel commented Oct 18, 2023

we have already a working function to clone config objects, so replacing

	const clonedDefinition = cloneObject(moduleDefinition);

with

	const clonedDefinition = configMerge(moduleDefinition);

works.

Will do more tests and provide a PR tomorrow.

khassel added a commit to khassel/MagicMirror that referenced this issue Oct 19, 2023
MichMich added a commit that referenced this issue Jan 1, 2024
## [2.26.0] - 01-01-2024

Thanks to: @bnitkin, @bugsounet, @dependabot, @jkriegshauser,
@kaennchenstruggle, @KristjanESPERANTO and @Ybbet.

Special thanks to @khassel, @rejas and @sdetweil for taking over most
(if not all) of the work on this release as project collaborators. This
version would not be there without their effort. Thank you guys! You are
awesome!

This release also marks the latest release by Michael Teeuw. For more
info, please read the following post: [A New Chapter for MagicMirror:
The Community Takes the
Lead](https://forum.magicmirror.builders/topic/18329/a-new-chapter-for-magicmirror-the-community-takes-the-lead).

### Added

- Added update notification updater (for 3rd party modules)
- Added node 21 to the test matrix
- Added transform object to calendar:customEvents
- Added ESLint rules for jest (including jest/expect-expect and
jest/no-done-callback)

### Removed

- Removed Codecov workflow (not working anymore, other workflow
required) (#3107)
- Removed titleReplace from calendar, replaced + extended by
customEvents (backward compatibility included) (#3249)
- Removed failing unit test (#3254)
- Removed some unused variables

### Updated

- Update electron to v27 and update other dependencies as well as github
actions
- Update newsfeed: Use `html-to-text` instead of regex for transform
description
- Review ESLint config (#3269)
- Updated dependencies
- Clock module: optionally display current moon phase in addition to
rise/set times
- electron is now per default started without gpu, if needed it must be
enabled with new env var `ELECTRON_ENABLE_GPU=1` on startup (#3226)
- Replace prettier by stylistic in ESLint config to lint JavaScript (and
disable some rules for `config/config.js*` files)
- Update node-ical to v0.17.1 and fix tests

### Fixed

- Avoid fade out/in on updateDom when many calendars are used
- Fix the option eventClass on customEvents.
- Fix yr API version in locationforecast and sunrise call (#3227)
- Fix cloneObject() function to respect RegExp (#3237)
- Fix newsfeed module for feeds using "a10:updated" tag (#3238)
- Fix issue template (#3167)
- Fix #3256 filter out bad results from rrule.between
- Fix calendar events sometimes not respecting deleted events (#3250)
- Fix electron loadurl locally on Windows when address "0.0.0.0" (#2550)
- Fix updatanotification (update_helper.js): catch error if reponse is
not an JSON format (check PM2)
- Fix missing typeof in calendar module
- Fix style issues after prettier update
- Fix calendar test (#3291) by moving "Exdate check" from e2e to
electron to run on a Thursday
- Fix calendar config params `fetchInterval` and `excludedEvents` were
never used from single calendar config (#3297)
- Fix MM_PORT variable not used in electron and allow full path for
MM_CONFIG_FILE variable (#3302)

---------

Signed-off-by: naveen <172697+naveensrinivasan@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Karsten Hassel <hassel@gmx.de>
Co-authored-by: Malte Hallström <46646495+SkySails@users.noreply.github.com>
Co-authored-by: Veeck <github@veeck.de>
Co-authored-by: veeck <michael@veeck.de>
Co-authored-by: dWoolridge <dwoolridge@charter.net>
Co-authored-by: Johan <jojjepersson@yahoo.se>
Co-authored-by: Dario Mratovich <dario_mratovich@hotmail.com>
Co-authored-by: Dario Mratovich <dario.mratovich@outlook.com>
Co-authored-by: Magnus <34011212+MagMar94@users.noreply.github.com>
Co-authored-by: Naveen <172697+naveensrinivasan@users.noreply.github.com>
Co-authored-by: buxxi <buxxi@omfilm.net>
Co-authored-by: Thomas Hirschberger <47733292+Tom-Hirschberger@users.noreply.github.com>
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
Co-authored-by: Andrés Vanegas Jiménez <142350+angeldeejay@users.noreply.github.com>
Co-authored-by: Dave Child <dave@addedbytes.com>
Co-authored-by: grenagit <46225780+grenagit@users.noreply.github.com>
Co-authored-by: Grena <grena@grenabox.fr>
Co-authored-by: Magnus Marthinsen <magmar@online.no>
Co-authored-by: Patrick <psieg@users.noreply.github.com>
Co-authored-by: Piotr Rajnisz <56397164+rajniszp@users.noreply.github.com>
Co-authored-by: Suthep Yonphimai <tomzt@users.noreply.github.com>
Co-authored-by: CarJem Generations (Carter Wallace) <cwallacecs@gmail.com>
Co-authored-by: Nicholas Fogal <nfogal.misc@gmail.com>
Co-authored-by: JakeBinney <126349119+JakeBinney@users.noreply.github.com>
Co-authored-by: OWL4C <124401812+OWL4C@users.noreply.github.com>
Co-authored-by: Oscar Björkman <17575446+oscarb@users.noreply.github.com>
Co-authored-by: Ismar Slomic <ismar@slomic.no>
Co-authored-by: Jørgen Veum-Wahlberg <jorgen.wahlberg@amedia.no>
Co-authored-by: Eddie Hung <6740044+eddiehung@users.noreply.github.com>
Co-authored-by: Bugsounet - Cédric <github@bugsounet.fr>
Co-authored-by: bugsounet <bugsounet@bugsounet.fr>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Knapoc <Knapoc@users.noreply.github.com>
Co-authored-by: sam detweiler <sdetweil@gmail.com>
Co-authored-by: veeck <michael.veeck@nebenan.de>
Co-authored-by: Paranoid93 <6515818+Paranoid93@users.noreply.github.com>
Co-authored-by: NolanKingdon <27908974+NolanKingdon@users.noreply.github.com>
Co-authored-by: J. Kenzal Hunter <kenzal.hunter@gmail.com>
Co-authored-by: Teddy <teddy.payet@gmail.com>
Co-authored-by: TeddyStarinvest <teddy.payet@starinvest.com>
Co-authored-by: martingron <61826403+martingron@users.noreply.github.com>
Co-authored-by: dgoth <132394363+dgoth@users.noreply.github.com>
Co-authored-by: kaennchenstruggle <54073894+kaennchenstruggle@users.noreply.github.com>
Co-authored-by: jkriegshauser <jkriegshauser@gmail.com>
Co-authored-by: Ben Nitkin <ben@nitkin.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants