Make features more declarative

[fregante]

Given the trouble we're having with ajaxed features and the increasingly-dynamic nature of GitHub, I'm starting to think we should have a better way to define features.

selector-observer gets us almost there, but it awaits Dom-ready so we can't use it all the time.

For example if we expect a tab to be added on the tab bar, I'd expect to write something like:

register({
	include: [isRepo],
	selector: 'xyz',
	action: 'append',
	ourClass: 'rgh-releases-tab'
	element: async () => <a href={…} class="rgh-releases-tab">Releases</a>,
})

And the feature manager would automatically look for selector, on isRepo, and append the element unless ourClass already exists at that location.

This basically means we we'll have to use MutationObserver with subtree: true… but oh well.

Ideally though, this would be even higher level:

register({
	location: 'repo-tab',
	element: async () => <a href={…}>Releases</a>,
})

With locations/repo-tab.tsx that knows exactly where that location appears (isRepo), which selector to use and whether to append/prepend/insertBefore/etc. It could even detect the tab with its own auto-generated class.

For features that don't just add an element, there could be additional onInit/onAdd/onRemove handlers.

There can also be a way to remove elements, like:

register({
	location: 'repo-sidebar',
	action: async () => { /*manually remove elements as usual*/},
})

The difference would be that the location definition would know how to handle the sidebar and know when to run action (e.g. when the sidebar is regenerated)

---

Too complicated? 😬

If anyone wants to try this, you can start prototyping outside features/index (no options, etc) just to see what it could look like and if it's feasible.

Technically this register function could also just live inside our regular init, even if it can't return false, that's ok:

features.add({
	init: () => {
		register(...)
	},
	deduplicate: 'disabled', // Has its own logic, this string disables ajaxing completely
	awaitDomReady: false, // Has its own logic
})

[fregante]

Or the location definitions could be functions that can be called directly, like

import repoTab from '../locations/repo-tab';

repoTab({
	append: async () => <a href={…} class="rgh-releases-tab">Releases</a>,
})

import repoSidebar from '../locations/repo-sidebar';

repoSidebar({
	update: () => {
		/* do generic stuff */
	}
})

The latter could replace most of our existing functions without too much trouble since it just takes care of the "on change" event while leaving the operations procedural.

[fregante]

Naive example implementation:

// locations/repo-tab.ts
const selector = '.UnderlineNav';
function repoTab({append, id}) {
	if (!isRepo()) {
		return false;
	}

	const sidebar = await elementReady(selector);
	if (!select.exists(id, sidebar) && append) {
		const element = await append();
		element.classList.add(id)
		sidebar.append(element);
	}
	
	const deduplicator = <rgh-repo-nav/>;
	sidebar.append(deduplicator);

	// Restart when reloaded
	onElementRemoval(deduplicator, () => repoTab({append, id}));
}

// locations/repo-sidebar
const selector = 'main .sidebar';
function repoSidebar({update}) {
	if (!isConversation()) {
		return false;
	}

	const sidebar = await elementReady(selector, {children: false});
	update();
	const deduplicator = <rgh-repo-sidebar/>;
	sidebar.append(deduplicator);

	// Restart when reloaded
	onElementRemoval(deduplicator, () => repoSidebar({update}));
}

[fregante]

This would basically move a lot of the logic from the feature files to the locations. Probably this won't reduce the code, but it would make it easier to group behavior by location rather than by feature, so that multiple features (if they exist), can reuse the logic

For example location: reactions would mean:

• hasComments -> onNewComments -> update
• isReleaseOrTag/isDiscussion -> MutationObserver -> update

They could also be nested, for example location: comment-dropdown would mean:

• location: comment -> await dropdown opening -> append

[fregante]

Unfortunately ain't nobody got time for that. Such a system would require a lot of research work and trial and error, as well as rewriting existing features and the system itself as we regularly reach its design limits.

I think selector-observer already gets us 50% there if used correctly. We can start migrating to using that exclusively instead of using custom events (unless they're really simple).

The only issue is that it awaits DOM ready, but for many features that's not an issue, and we could look for an alternative library without such limitation if necessary.

[kidonng]

Unfortunately ain't nobody got time for that.

Why not just keep the issue open for a bit longer then?

That said, there indeed isn't a huge need to design a full-blown system and rewrite most features since few features target the same component/location, which means we don't have many chance to reduce duplicate logic.

The repo-tab example you provide is valid though, maybe it's useful to make a helper for that.

[fregante]

I just don't think it's worth going too far off the current loading setup if we don't plan on going all in. I don't want to have just a few features that handle loading completely differently.

I'm not a fan of keeping "Let's rewrite the core" issues open because nobody except current maintainers will pick them up. We can still discuss it though and exploratory PRs can still be opened.

Generally though I think that there aren't many features that can be generalized this way. Sometimes we have nested ajaxed elements so even if I could target repo-sidebar nothing is still stopping GitHub from adding an ajaxed component inside and breaking all of our deduplication logic. Only specific selector-observer calls really can do that.

If there are reusable pieces, we can wrap whatever mechanism we have in functions like we're already doing: https://github.com/sindresorhus/refined-github/blob/main/source/github-events/on-pr-merge-panel-open.ts