Skip to content

Added outgoing notification of the weather module #2842

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.

Sign up for GitHub

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

Merged
MichMich merged 5 commits into from
May 18, 2022
Merged

Added outgoing notification of the weather module #2842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
df0f048
Added outgoing notification
eouia Apr 26, 2022
58569a6
Added ECMA Version of lint to 2020
eouia Apr 26, 2022
038b676
Change property name clearly
eouia Apr 26, 2022
0f39b77
fix typo jsdom tag returns
eouia Apr 26, 2022
1d90c5e
Add JSDoc description
eouia Apr 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
},
"parserOptions": {
"sourceType": "module",
"ecmaVersion": 2018,
"ecmaVersion": 2020,
"ecmaFeatures": {
"globalReturn": true
}
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ _This release is scheduled to be released on 2022-07-01._

### Added

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

@KristjanESPERANTO KristjanESPERANTO May 9, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: information


### Updated

### Fixed
Expand Down
9 changes: 9 additions & 0 deletions modules/default/weather/weather.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,15 @@ Module.register("weather", {
if (this.weatherProvider.currentWeather()) {
this.sendNotification("CURRENTWEATHER_TYPE", { type: this.weatherProvider.currentWeather().weatherType.replace("-", "_") });
}

const notificationPayload = {
currentWeather: this.weatherProvider?.currentWeatherObject?.simpleClone() ?? null,
forecastArray: this.weatherProvider?.weatherForecastArray?.map((ar) => ar.simpleClone()) ?? [],
hourlyArray: this.weatherProvider?.weatherHourlyArray?.map((ar) => ar.simpleClone()) ?? [],
locationName: this.weatherProvider?.fetchedLocationName,
providerName: this.weatherProvider.providerName
};
this.sendNotification("WEATHER_UPDATED", notificationPayload);
},

scheduleUpdate: function (delay = null) {
Expand Down
17 changes: 17 additions & 0 deletions modules/default/weather/weatherobject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,23 @@ class WeatherObject {
this.sunrise = moment(times.sunrise, "X");
this.sunset = moment(times.sunset, "X");
}

/**
* Clone to simple object to prevent mutating and deprecation of legacy library.
*
* Before being handed to other modules, mutable values must be cloned safely.
* Especially 'moment' object is not immutable, so original 'date', 'sunrise', 'sunset' could be corrupted or changed by other modules.
*
* @returns {object} plained object clone of original weatherObject
*/
simpleClone() {

@rejas rejas Apr 26, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eouia eouia Apr 26, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sdetweil sdetweil Apr 26, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.

@eouia eouia Apr 26, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rejas rejas Apr 26, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@rejas rejas Apr 26, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rejas rejas Apr 26, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.

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)?

@eouia eouia Apr 26, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eouia eouia Apr 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

@rejas rejas May 1, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

const toFlat = ["date", "sunrise", "sunset"];
let clone = { ...this };
for (const prop of toFlat) {
clone[prop] = clone?.[prop]?.valueOf() ?? clone?.[prop];
}
return clone;
}
}

/*************** DO NOT EDIT THE LINE BELOW ***************/
Expand Down