Skip to content

add error handling to weather fetch functions, including cors #3791

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

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

sdetweil
Copy link
Collaborator

add error detection and handling for weather and cors fetch

fixes #3687

@sdetweil sdetweil added the draft label May 26, 2025
@sdetweil
Copy link
Collaborator Author

all tests run locally, electron and e2e

@khassel
Copy link
Collaborator

khassel commented May 26, 2025

all tests run locally, electron and e2e

the 2 failed tests are unit tests - and they fail if I run them locally ...

@jbaboval
Copy link

jbaboval commented Jun 7, 2025

Running this patch locally with success thus far.... Thanks!

if (type === "xml") {
return new DOMParser().parseFromString(data, "text/html");
} else {
if (!data || !data.length > 0) return "null"; //undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Return "null" as a string looks unusual. Can you explain why you did this and why there is a comment // undefined?

Copy link
Collaborator Author

@sdetweil sdetweil Jun 8, 2025

Choose a reason for hiding this comment

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

Looking at suggested error recovery, the undefined prior result (comment) is faulty
Leading to a secondary error masking the first (no data)

And null in quotes is the recommended change

I don't understand that (or undefined) either, as none of these are JSON , or error (throw)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and looking at the provider that calls the performWebRequest (here looking at pirateweather)

	fetchWeatherForecast () {
		this.fetchData(this.getUrl())
			.then((data) => {
				if (!data || !data.daily || !data.daily.data.length) {  // undefined or null (no quotes) 
					// No usable data?
					return;  // <----- I think this bypasses the finally, so the provider is waiting forever, hung
				}

				const forecast = this.generateWeatherObjectsFromForecast(data.daily.data);
				this.setWeatherForecast(forecast);
			})
			.catch(function (request) {   // <--- maybe throw is a better choice
				Log.error("Could not load data ... ", request);
			})
			.finally(() => this.updateAvailable());

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the fetchData in the providers layer

	async fetchData (url, type = "json", requestHeaders = undefined, expectedResponseHeaders = undefined) {
		const mockData = this.config.mockData;
		if (mockData) {
			const data = mockData.substring(1, mockData.length - 1);
			return JSON.parse(data);
		}
		const useCorsProxy = typeof this.config.useCorsProxy !== "undefined" && this.config.useCorsProxy;
		return performWebRequest(url, type, useCorsProxy, requestHeaders, expectedResponseHeaders, config.basePath);
	}

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jun 9, 2025

Every other provider had to be fixed as well

@KristjanESPERANTO
Copy link
Collaborator

KristjanESPERANTO commented Jun 9, 2025

@sdetweil I worked on a solution and pushed it directly into your branch. I hope that's okay(?) Unfortunately, I was not able to provoke the error before and after this PR, but the tests are now running again. Can you check if the error occurs with it?

@sdetweil
Copy link
Collaborator Author

sdetweil commented Jun 9, 2025

I saw. Will try to test today. I had forced the error path
Was trying not to change the providers

@KristjanESPERANTO
Copy link
Collaborator

@sdetweil What do you think? Can we merge this?

@sdetweil
Copy link
Collaborator Author

let me do some testing, been pretty busy lately

@KristjanESPERANTO
Copy link
Collaborator

No problem. Take your time 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants