search

material-extensions / material-icons-browser-extension

Browser Addon that enhances file browsers of version controls with material icons.
MIT License
524 stars 39 forks source link

Why is rushFirst used? #100

Closed mikeydevelops closed 2 months ago

mikeydevelops commented 2 months ago

So, I have been trying to debug #80 and by extension #91. Found out that when you have Material Icons installed with Refined GitHub, and when scrolling while the page is loading, some icons do not change. I traced it to the rushFirst function. https://github.com/material-extensions/material-icons-browser-extension/blob/5a9602e5a33878167e0de90bae56f77855b98029/src/lib/replace-icons.ts#L8-L22 The function came in 7a2148e https://github.com/material-extensions/material-icons-browser-extension/blob/7a2148e9cc7843e02a9c9825e0f5a5c5f29efeee/src/main.js#L21-L41 Here it says replacing synchronously prevents blinks, but those blinks only happen when using Refined GitHub.

By the way, converting to typescript messed up git history. When seeing history for file src/lib/replace-icons.ts it says is says Renamed from src/providers/azure.js

Anyway, so when replacing:

rushFirst(90, callback);

with only

callback();

in https://github.com/material-extensions/material-icons-browser-extension/blob/5a9602e5a33878167e0de90bae56f77855b98029/src/lib/replace-icons.ts#L24-L33

Everything works fine in large repositories even while scrolling while the page is loading.

I think @Claudiohbsantos was developing the extension with Refined GitHub on and that introduced the bug. I think we should change the function call to just callback() and leave it at that. It is not this extension's fault that the icons are flashing. That is coming from Refined GitHub. They should change the DOM once, but it appears they are doing it multiple times.

What do you think?

mikeydevelops commented 2 months ago

I just saw, @theoludwig fixed it in their own fork. And they fixed it the same way I did in https://github.com/theoludwig/github-material-icons-extension/commit/a4977601ea8f947834ad5cac12c7c5805a78505a

PKief commented 2 months ago

What do you think?

I'm agreeing with you. We should remove this function if it's only related to the refined Github extension. And it's worth to remove it especially if it resolves that bug. Personally, I also think that this function looks quite hard to understand and to maintain as it is right now.