New weather provider: Yr.no #2948
Conversation
MagMar94
changed the title
The Yr documentation was quite strict on using a cache together with the expires- and if-modified since-headers. As a result the weather provider has to send and recieve HTTP-headers. We use the default User-Agent for now, even though it is on a slightly different format than the one documented by Yr.
MagMar94
force-pushed
the
feature/weather-from-yr
branch
from
3b25f9f to
90bcc1d
Compare
|
When testing the yr weather provider I got a sense that the synchronization inspired by this article wasn't working, so I added a few log-statements like this: getWeatherDataSynchrounous() {
console.log("Getting weather data. Waiting: " + this.waitingForWeatherData + ". Queue: " + this.weatherDataQueue.length);
...
if (!this.waitingForWeatherData) {
this.waitingForWeatherData = true;
console.log("Locked weather data.");
...
if (this.weatherDataIsValid(weatherData)) {
console.log("Unocked weather data.");
this.waitingForWeatherData = false;
this.getWeatherDataSynchrounous();
Log.debug("Weather data found in cache.");
...
} else {
this.getWeatherDataFromYr(weatherData?.downloadedAt)
.then((weatherData) => {
Log.debug("Got weather data from yr.");
...
})
.catch((err) => {
...
})
.finally(() => {
console.log("Unocked weather data.");
this.waitingForWeatherData = false;
this.getWeatherDataSynchrounous();
});
}
}
},The output proved me right (disregard the error, I changed the The problem roots in that the weather module instances are separate classes. This obviously results in them having individual class variables. I have tree options:
I'm not sure how I can make a generic cache in the server, because I'm not sure how to implement generic logic for how long to keep the cache. Specifically for the Yr-provider it should be kept as long as it is valid (a time) or if after the expiration time, the server returns a status code indicating the data haven't changed. For option three I would affect a lot of modules that does not require synchronization. That leaves option two. I'm attempting to implement this now. |
Adds support for sending and receiving HTTP-headers when using the CORS-method. This change is required for the Yr weather-provider introduced in #2948. To make it easier to add unit tests I moved the server-functions into a separate file.
If a user has multiple instances of the same weather-module, this may lead to multiple calls to the API. This local storage-based synchronization is an attempt to avoid that.
MagMar94
changed the title
Codecov Report
@@ Coverage Diff @@
## develop #2948 +/- ##
===========================================
- Coverage 24.00% 22.96% -1.05%
===========================================
Files 49 51 +2
Lines 10121 10876 +755
===========================================
+ Hits 2430 2498 +68
- Misses 7691 8378 +687
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
The current implementation is faulty. I think it is useless as well, because mirrors are likely to only show weather for one location. If this is to be implemented, it should be done for both weather and stellar data.
MagMar94
force-pushed
the
feature/weather-from-yr
branch
from
c32d122 to
5fb1fd1
Compare
Because the fetch used requires a later node version, I only test the ones that work. The others gets a default test to avoid empty test suite.
modules/default/utils.js
Outdated
| return responseHeaders; | ||
| }; | ||
|
|
||
| module.exports = { |
There was a problem hiding this comment.
This isnt recognized by the brwoser and leads to
"Uncaught ReferenceError: module is not defined
at utils.js:144:1
"
There was a problem hiding this comment.
strange, "works on my machine"... I'll fix it 👍
There was a problem hiding this comment.
How about now? I used the if (typeof module !== "undefined")-check used in several other parts of the code.
Avoid crashing, just trim the coordinates. I decided to trim instead of round for simplicity.
| }); | ||
| }); | ||
| } else { | ||
| test("Always ok, need one test", () => {}); |
There was a problem hiding this comment.
so why exactly dont we need a test for node < 18 ?
There was a problem hiding this comment.
Because they don't run 👀 It boils down to the fetch-method (in much the same way as I struggled with fetch in #2957 (comment)). When I tested with node 16.17.1 I got the following error:
It seems the fetch-related types are not defined. According to the docs fetch was introduced in Node 18, so it makes sense. Fetch was introduced to the weather module in #2935. I figured since fetch was already implemented I wouldn't bother with reimplementing another kind of fetch.
|
Thx for your work!! |
Added Yr.no as a weather provider
Yr.no is a free Norwegian weather service. The configuration is quite simple:
The latitude and longitude cannot have more than 4 decimals, but that should be plenty. To quote yr: "There is no need to ask for weather forecasts with nanometer precision!". The altitude should be meters above sea level and defaults to 0. If
typeis set tocurrentthe symbol can display the next 1, 6 or 12 hours by settingcurrentForecastHours(default is 1).It states in Getting started-guide that users of the API should cache the results and use the
Expires-header to know when to ask for new data. By using theIf-Modified-Since-header we can avoid downloading the same data over and over again. I chose not to override theUser-Agent-header set inserver.jseven though it does not comply with the terms of service. It currently works with the default header, and by searching the web for MagicMirror the GitHub-repo should be easy to find without an explicit link.I also had to make some minor changes to
server.jsandweatherprovider.jsto be able to send and return HTTP headers. To handle the HTTP 304 response without body I chose to returnundefinedso we easily can use the response as a condition:if (response) ....The documentation for the API is available here: