Explore using `selector-observer` to run features #3084

Many of the bugs currently open can be fixed with this, so I really hope that someone can figure it out.

Let’s avoid any more conversions to selector-observer for the time being. There are some outstanding drawbacks we need to solve first. For example:

• it always automatically waits for domready, so it’s not as fast as one using element-ready.
• sometimes it should be disabled on deinit, like for that recent feature to remove labels.
• some features should only be applied when the elements are visible (I have some code about this in the first related PR)

We should find a common, efficient way to cover all of these issues. Either via code or via documentation (e.g. “if feature requires X, then you must use deinit”)

cc @yakov116 @kidonng

• it always automatically waits for domready, so it’s not as fast as one using element-ready.

I don't have a strong feeling about this so I can't say anything.

• sometimes it should be disabled on deinit, like for that recent feature to remove labels.

I think most of the time this could be solved with carefully chosen selectors.

• some features should only be applied when the elements are visible (I have some code about this in the first related PR)

Are you referring to this one? May worth try implementing it again, but I can't think of a feature that could make use of it for the moment.

I can see you are continuing the discussion in #3514, I'm also aware of the issue but didn't jump in yesterday because my thoughts are not clear. Now I'll just put in my two cents: I'm not familiar with all existing and pending usage of selector-observer, but I would say it is definitely useful for dealing with ajaxed components. For sure we don't want to convert every feature into using it, but I think our original intention is to fix bugs which will be hard/verbose without it. Take my two open PR converting into selector-obsever as an example, I think there's a good reason to use it. Existing usage like linkify-user-edit-history-popup and remove-label-faster may be rewritten to reduce the usage of selector-observer (judge from a glance, correct me if I'm wrong). In short, if a feature works well, we don't need to use selector-observer. If the code is nice without selector-observer, we can remove the use of it. If a feature has a bug that selector-observer can solve with ease, we should favor it. This is just my own humble opinion though.

I think most of the time this could be solved with carefully chosen selectors.

That may be right. Do you want to try reverting https://github.com/sindresorhus/refined-github/pull/3476?

Are you referring to this one

Yes, I used that function for a couple of features. But yes, it's probably not useful in many cases.

It doesn't need to be rewritten, it worked well, it only lacked :not(.rgh-class) but I did not have time to investigate back then.

Existing usage like linkify-user-edit-history-popup and remove-label-faster may be rewritten to reduce the usage of selector-observer (judge from a glance, correct me if I'm wrong).

I think those are exactly the features that are fine with selector-observer

• linkify-user-edit-history-popup needs complicated listeners for the popup, and doesn't benefit from element-ready because it appears well after dom-ready
• remove-label-faster affects the sidebar, which is:
  • a ajaxed "hot zone"
  • loaded last on the page, so element-ready wouldn't save a lot of FOUC

In short, if a feature works well, we don't need to use selector-observer

Only until we sort this out. There are some features that are a mess of listeners and could benefit from selector-observer’s simplicity.

What do you think about this? 👇

<details> <summary>Edit: maybe too wordy</summary>

Always use selector-observer when...

... the feature affects a number of elements across the page and they're in ajaxed "hot zones" e.g. parse-backticks can find elements to modify anywhere on the page. element-ready would not have any advantages here.

Always use element-ready when...

... the feature only affects a specific part of the page and it's not in an ajaxed "hot zone" (e.g. conversation header and sidebar, and list of files)

For the rest of them...

... perhaps select.all is enough ... we need to evaluate whether it's worth giving up "faster features" (https://github.com/sindresorhus/refined-github/issues/2671) in exchange for reliability.

</details>

I'll try rewriting that:

"Ajaxed hot-zones" are areas that have nested ajaxed widgets and require a number of listeners and mutation observers to detect changes.

1. Features in ajaxed hot-zones or that need complicated listeners/MutationObservers: Use selector-observer
2. Features with a limited number of elements, e.g. forked-to only affects links in the header: Use element-ready
3. Features with many elements across the page: Use select.all

Urgent debugging wanted

• Is https://github.com/josh/selector-observer/issues/30 a real bug?
• Did https://github.com/sindresorhus/refined-github/issues/3527 happen for that reason?
• How can we work around it?

Further features and refactoring using selector-observer won't be accepted until we know how to avoid this from happening.

cc @kidonng @yakov116 this is why I temporarily closed related PRs.

https://github.com/kylekyle/monitoring what about this? It's actively maintained and it looks like it runs before dom ready too!

Another good thing is.

Finally, you can cancel by returning false from within the callback. Note: Your callback must return false, not a falsey value like null or undefined.

Which means we can stop the observer on some features after all the elements loaded.

@fregante should I try https://github.com/kylekyle/monitoring and play around with it? Or do you want to stick to select-observer?

We need to verify those 3 points first, there's no point in changing library if has the same issue (or if the issue doesn't exist)

I confirmed the bug and found a workaround:

• if the selector has :not(.rgh-this-feature) and we add the class right away, selector-observer will no longer call the add method because the original selector no longer applies. This avoids the loop.

We almost always do that already, but #3517 didn't because it didn't seem necessary.

• I'll open the previously-locked PRs
• @kidonng you can resubmit #3517 with the workaround described #3531 already exists 👍
• @yakov116 if you want to explore Monitoring anyway, try it out on linkify-code, which is the heaviest feature making use of selector-observer

@yakov116 if you also want to look into any other features that use selector-observer but don't have this class, please send a PR for that