Added outgoing notification of the weather module by eouia · Pull Request #2842 · MagicMirrorOrg/MagicMirror

eouia · 2022-04-26T12:00:10Z

The current weather module seems no notification emission.

Added WEATHER_UPDATED notification when new weather information is provided.

When this PR is accepted, I'll modify wiki page of MM manual.

codecov-commenter · 2022-04-26T12:02:54Z

Codecov Report

Merging #2842 (df0f048) into develop (439690b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2842   +/-   ##
========================================
  Coverage    65.15%   65.15%           
========================================
  Files            8        8           
  Lines          287      287           
========================================
  Hits           187      187           
  Misses         100      100

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 439690b...df0f048. Read the comment docs.

eouia · 2022-04-26T12:16:49Z

Hmmm... The optional chaining(?.) made the test fail.
Optional chaining has been adopted as standards since NodeJS 14 and ECMA 2020 (Chrome 80 and all the current stable browsers.)

Is this not acceptable?

rejas · 2022-04-26T12:17:31Z

modules/default/weather/weather.js

+			forecast: this.weatherProvider?.weatherForecastArray?.map((ar) => ar.simpleClone()) ?? [],
+			hourly: this.weatherProvider?.weatherHourlyArray?.map((ar) => ar.simpleClone()) ?? [],
+			location: this.weatherProvider?.fetchedLocationName,
+			provider: this.weatherProvider.providerName


maybe also call it "providerName" to avoid misunderstanding

good point. I'll modify it.

rejas · 2022-04-26T12:21:06Z

Hmmm... The optional chaining(?.) made the test fail. Optional changing has been adopted as standards since NodeJS 14 and ECMA 2020 (Chrome 80 and all the current stable browsers.)

Is this not acceptable?

It snot node but eslint that fails due to the optional chaining. As can be read here:
https://eslint.org/blog/2020/07/eslint-v7.5.0-released
you need to use

    "ecmaVersion": 2020

eouia · 2022-04-26T12:55:17Z

Hmmm... The optional chaining(?.) made the test fail. Optional changing has been adopted as standards since NodeJS 14 and ECMA 2020 (Chrome 80 and all the current stable browsers.)
Is this not acceptable?

It snot node but eslint that fails due to the optional chaining. As can be read here: https://eslint.org/blog/2020/07/eslint-v7.5.0-released you need to use
    "ecmaVersion": 2020

Than ks for hints.

rejas · 2022-04-26T14:13:52Z

modules/default/weather/weather.js

 			this.sendNotification("CURRENTWEATHER_TYPE", { type: this.weatherProvider.currentWeather().weatherType.replace("-", "_") });
 		}
+
+		let notificationPayload = {


can be a const

rejas · 2022-04-26T14:15:16Z

modules/default/weather/weatherobject.js

+	 *
+	 * @returns {object} simple object cloned.
+	 */
+	simpleClone() {


since there are so many js features used in this method (which might confuse beginners) would be nice to have an example of what this does in the jsdoc.

I'm not sure a good example could be possible because this feature is only for making a safe-immutable payload of notification.
Anyway, I added some description more.

@eouia @rejas all the default modules send notifications of their data so that others might compose new apps..
BUT since the new weather module combined current weather and forecast, it (the new module) does NOT broadcast the forecast info when running in forecast mode.

By the way, all the payload of original default modules is mutable and not safe from modifying by recipient modules I think that could be a risk in some cases.

By the way, all the payload of original default modules is mutable and not safe from modifying by recipient modules I think that could be a risk in some cases.

so thats why you added this method?

Also: I am wondering if there isnt a lodash method that would do the same thing?

@eouia @rejas all the default modules send notifications of their data so that others might compose new apps.. BUT since the new weather module combined current weather and forecast, it (the new module) does NOT broadcast the forecast info when running in forecast mode.

yes, it only published the CURRENTWEATHER_TYPE, so that the compliments module can react to it ( I think I added that back in the days) Maybe its a good idea to remove that call and broadcast only the whole weather object (instead of just the CURRENTWEATHER_TYPE)?

moment object is not immutable and unsafe.

For example; In case of ‘weatherObject.sunrise’ is handed to other module as payload directly, the 3rd party module can manipulate the object;

/* In other module’s receivedNotification function */ let afterSunRise = payload.sunrise.add(30, ‘minute’)

But by this execution, the original this.weatherProvider.currentWeatherObject.sunrise Will be changed. Usually this is a bad side-effect unintentional.

And I prefer pure JS itself as far as possible, instead of being dependent on 3rd party library in the browser.
Different with usual nodeJS app, MM on the browser is not modular. All the 3rd party libraries are loaded get global scope. It is not comfortable for me with that. :)

I dont mind 3rd party libraries like lodash, sicne they are well tested and we already use it in here. But your code your choice :-)

KristjanESPERANTO · 2022-05-09T15:43:34Z

CHANGELOG.md


 ### Added

+- Added the notification emitting from the weather module on infromation updated.


Typo: information

Added outgoing notification

df0f048

rejas reviewed Apr 26, 2022

View reviewed changes

eouia added 2 commits April 26, 2022 14:51

Added ECMA Version of lint to 2020

58569a6

Change property name clearly

038b676

fix typo jsdom tag returns

0f39b77

rejas reviewed Apr 26, 2022

View reviewed changes

Add JSDoc description

1d90c5e

rejas approved these changes May 1, 2022

View reviewed changes

KristjanESPERANTO reviewed May 9, 2022

View reviewed changes

MichMich merged commit 8e5267d into MagicMirrorOrg:develop May 18, 2022


		### Added

		- Added the notification emitting from the weather module on infromation updated.

Uh oh!

Conversation

eouia commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 26, 2022

Codecov Report

Uh oh!

eouia commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rejas commented Apr 26, 2022

Uh oh!

eouia commented Apr 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eouia Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

eouia commented Apr 26, 2022 •

edited

Loading

eouia commented Apr 26, 2022 •

edited

Loading

eouia Apr 26, 2022 •

edited

Loading