Use Preact?

[fregante]

Would it make sense to drop dom-chef and just go straight to Preact?

Downsides:

• heavier (only +10/15 KB though)
• slower (given the amount of DOM change, how much really?)
• requires rewrite of many parts that aren't just .append(<SomeJSX/>)

Upsides:

• no more type issues
• no more DOM spaghetti
• components that require DOM updates will be easier to handle
• ease of contribution since Preact is known and well documented, unlike the alternatives
• we can use @primer/components directly, which could reduce the amount of JSX we carry

We can try converting features gradually to test the feasibility and see what changes it brings. First from the simplest feature, then from a feature that would actually benefit from using Preact.

This will likely conflict with our existing JSX types, so we might have to clutter the codebase with ts-expect-error or just add || true in the build:typescript npm script 😅

[busches]

Why do you think it'll be slower? Just the overhead we're adding that we won't use? I initially thought we'd get a speed up for the DOM, but maybe DOM Chef is really speedy.

[fregante]

Why do you think it'll be slower?

• dom-chef can't do any fewer operations than it does
• preact does a lot more than dom-chef (as demonstrated by the 3x code increase)

Both start from pre-parsed JSX; One ends up with plain DOM nodes while the other needs to keep track of them and diff them once we ask to change data.

Our "change of data" currently directly changes the data on the DOM element. Our only overhead in this case is that sometimes we call select to get the DOM element again.

Again I don't think this is substantially slower, but I don't doubt that it is at least a little bit slower.

[fregante]

I’m more concerned about the third downside since it requires some work + I still don’t know how to manage things like .before(<div/>) in Preact since it can only append to a root node. This is a lot of code: https://stackoverflow.com/a/57899813

[sindresorhus]

I'm a little bit doubtful it's worth the effort. So I'm -0 on this.

[fregante]

I think it mostly depends on the code it requires. If it ends up making it longer and more complex, no. For the append/insertBefore I can write some helpers so it will still be as easy as before(‘selector’, <div/>)

[notlmn]

Our "change of data" currently directly changes the data on the DOM element. Our only overhead in this case is that sometimes we call select to get the DOM element again.

If we are to query DOM elements that we created in JSX we already use, then we don't need select for them. select is only used for elements that are outside our JSX scope, and that belongs to the DOM generated GitHub. Is'nt that the case?

---

I really see an upside to DOM diffing instead of root.innerHTML = <App /> or something like that. But we still have to use MutationObservers all over the place, that's again because most of the DOM is outside our JSX scope. This comes down to how each feature ends up using DOM diffing or other features of Preact.

[fregante]

Our "change of data" currently directly changes the data on the DOM element. Our only overhead in this case is that sometimes we call select to get the DOM element again.

If we are to query DOM elements that we created in JSX we already use, then we don't need select for them.

Check this out: https://github.com/sindresorhus/refined-github/blob/5288b97cff1e84e2f7d251ddcdefe18bd5ab7c59/source/features/conversation-activity-filter.tsx#L102

I select them again so that I don’t have to keep track of the 2 instances of the same widget. Really this is the shortest way to keep them in sync, but obviously it’s really fragile.

This is also the same feature that would benefit from changing state in a single place, instead of changing it in 3 places and then trying to update the UI accordingly, also via CSS (the open/closed eye icon is handled via CSS selector; spaghetti all the way)

That would be replaced with this, in the main JSX, without having to add code to update it later since we only change “setting” once:

{setting === 'default' ? <Eye/> : <EyeClosed/>}

But we still have to use MutationObservers all over the place, that's again because most of the DOM is outside our JSX scope.

That’s right, that part is unaffected though. This issue is about replacing dom-chef, not attempting to rewrite the whole extension as a SPA. Each JSX piece will be treated as its own interactive widget and injected as before into the DOM.

[fregante]

Requires too much work. Never mind. If I could use it just on some features it’d good, but typescript makes that impossible.

[fregante]

Spaghetti it is. Enjoy. 🍝