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

feat: Support icon alias #685

Merged
merged 2 commits into from
Dec 9, 2024
Merged

feat: Support icon alias #685

merged 2 commits into from
Dec 9, 2024

Conversation

ramedina86
Copy link
Collaborator

No description provided.

@ramedina86 ramedina86 merged commit f663ac2 into dev Dec 9, 2024
19 checks passed
Comment on lines +145 to +152
async function checkIfUrlExists(url: string) {
try {
const response = await fetch(url, { method: "HEAD" });
return response.ok;
} catch {
return false;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't you want to cache it to avoid sending the same request for every same node ?

It could be done easily by moving this in a separate TS file

const cacheUrlExists = new Map<string, boolean>();

export async function checkIfUrlExists(url: string) {
    const cache = cacheUrlExists.get(url);
    if (cache !== undefined) return cache;

	try {
		const response = await fetch(url, { method: "HEAD" });
        cacheUrlExists.set(url, true);
		return response.ok;
	} catch {
        cacheUrlExists.set(url, false);
		return false;
	}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good improvement, you're right. I was thinking of doing a composable as there is very similar logic in the Toolkit that I want to unify. I'll add cache. Or if you want to handle it...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to assign me a task and I'll take care of it

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

Successfully merging this pull request may close these issues.

2 participants