guardian / frontend

The Guardian DotCom.
https://theguardian.com
Other
5.83k stars 555 forks source link

sunsetting 3rd party DOM microlibs #17467

Closed sndrs closed 5 years ago

sndrs commented 7 years ago

Should we remove bonzo, bean and qwery? why? do we replace them with something else?

@joelochlann has proposed #17461 (see also https://github.com/guardian/frontend/pull/17425#issuecomment-315403295)

(I've assigned @guardian/dotcom-platform client side + people who've already expressed an interest – but this is open to anyone who wants to contribute).

sndrs commented 7 years ago

this is basically the discussion we've had so far (at the least the one that spurred this)

@joelochlann said:

Having stripped out most of the usages of $ in acquisitions-epic-liveblog.js without any cost, it occurred to me that the only things significantly more verbose without $ were $.create() and .insertAfter()

So I wondered whether in general it might be best to have a handful of our own library functions (which we can then add Flow types to) rather than the bonzo/bean/qwery combination, none of which have been maintained for years and which have been made largely redundant by advances in the native DOM APIs plus polyfill.io

@sndrs said:

removing those is absolutely something we want to do, for the pretty much the same reasons you outlined there.

we decided against ditching them during es6 conversion because it seemed too much for convertors to take on (we already banned unnecessary lodash methods, for example). at the same time, we considered old es5 code to be frozen, so didn't really want to update that too.

personally i'd be keener to see us use something like preact to remove any manual patching of the DOM completely. that is obviously a much bigger job though, and may be too puritan to be useful. that's just one possible approach though, your suggestion another – there will likely be a lot to talk about before deciding how to proceed after their removal.

the reason we've left those micro libs in place currently is that they at least standardise our code, are well battle-tested and for the moment avoid us essentially re-creating them in manifold private ways across modules.

@gustavpursche said:

Just my 5ct: I'd like to rather not start with this now until we have a longer discussion about the future we want. It feels a bit, that we'd invent a second bonzo over time, if we start to wrap everything into dom.js. I like to encourage people to use native stuff and to provide helpers, for the hard things (event delegation, inserts and create). Also right now I don't really see the problem it would solve - as part of the ES6 conversion we got rid of bonzo in many places and replaced bean by native bindings.

This is an effort which could continue, so that in the end we only have a handful of problemats, that we could solve with a helper like you proposed, but much more specialized.

philwills commented 7 years ago

The reason I would give in support of removing bonzo, based on my experience with ES6 migration, is that bonzo hasn't been designed with types in mind, so wherever it appears, Flow's utility drops significantly.

I'm not confident that's a compelling reason to change, but it's the main one that would dissuade me from using bonzo given the option.

SiAdcock commented 7 years ago

As a tangential consideration, if we are to drop qwery, etc, we need to ensure we always convert NodeLists to proper arrays so that we are able to use iterator methods such as forEach, reduce, etc on the resulting selection.

This is a significant problem at the moment because Flow assumes NodeLists always have iterator methods, whereas polyfill.io does not provide these methods for browsers that have chosen not to supply them natively.

GHaberis commented 7 years ago

My instinct is to remove these and replace with native implementations. However I do agree that if we're gong to start using something like preact (for example) that assumes responsibility for touching the DOM then perhaps there's an argument that we should be discussing this rather than revisiting existing implementations of these libs.

There are elements of those lib's APIs that are useful (eg. bean implements an easy interface for event delegation) however we could write our own helpers for these. I'm sure if we start converting to native we'd find there aren't many scenarios where it's much more effort to use native alternatives to the lib, and if we do come across them we can write our own helpers. That'd be preferable to importing an entire lib just to use a handful of it's functions.

There's always the issue that if a libs available people will use it, even if it's unnecessary and an equivalent native solution would have been as easy to implement.

GHaberis commented 7 years ago

@SiAdcock definitely agree about the NodeList to Array conversion, spread operators have made this incredibly easy now at least! [...NodeList] make me very happy!

joelochlann commented 7 years ago

As a tangential consideration, if we are to drop qwery, etc, we need to ensure we always convert NodeLists to proper arrays so that we are able to use iterator methods such as forEach, reduce, etc on the resulting selection.

@SiAdcock yep, I had that in my dom.js prototype:

export const elementsFromSelector = (selector: string): Array<Element> => [
    ...document.querySelectorAll(selector),
];
regiskuckaertz commented 7 years ago

ha! frontend's arch-Nemesis 😆 my opinion:

then there are other things, like these libs are supposed to make our lives easier, but seeing that their type signatures are not compatible (esp bean), that is not true. Then there's also the myriad of use cases (try figuring out the signature of qwery alone!) we never use, which led to performance sacrifices in the implementation. 👻

regiskuckaertz commented 7 years ago

I forgot to add that there is an obvious advantage to using these APIs: the cognitive load of memorising standard DOM APIs, their long names coming from an dystopian future where Java developers rule the world, and their inconsistent signatures (NodeList vs HTMLCollection, live collections or not, what the hell?) ... I mean, there are more important things to do in real life. Even the simple fact that one should remember to use the spread operator is, in my opinion, de trop.

LATaylor-guardian commented 7 years ago

I forgot to add that there is an obvious advantage to using these APIs: the cognitive load of memorising standard DOM APIs, their long names coming from an dystopian future where Java developers rule the world, and their inconsistent signatures (NodeList vs HTMLCollection, live collections or not, what the hell?) ... I mean, there are more important things to do in real life. Even the simple fact that one should remember to use the spread operator is, in my opinion, de trop.

A BIG advantage in my opinion.

joelochlann commented 7 years ago

their long names coming from an dystopian future where Java developers rule the world

Which are the worst offenders these days? Seems like a lot of stuff is not that much longer in the native way than the bonzo way (.setAttribute() vs .attr(), .addClass() vs classList.add()).

You make a very good point about .getElementByClassName() etc and the fact that qwery will choose the best performing implementation (didn't realise how much slower querySelectorAll() is), but as you say, we could write something tiny ourselves to handle that.

their inconsistent signatures (NodeList vs HTMLCollection, live collections or not, what the hell?)

Presumably however we choose to bypass the verbosity/performance trade-offs of .getElementByClassName() vs querySelectorAll() would also output an array so we wouldn't need to worry about that one.

There's a certain cognitive load to learning the bonzo API too - a lot smaller, for sure, but then the knowledge is less widely applicable. The thing I found most annoying when I started using it was that it replicated most of the jQuery API, but not all of it - so it lulled me into a false sense of security but then I found myself getting annoyed at the missing bits (.closest() is only in the "Ender bridge" which we don't have, .find() is missing, when I reached for .on() I realised I needed bean for that, etc). I guess these are pretty minor points with very easy workarounds, but personally I'd rather know the quirks of the DOM than the quirks of an unmaintained library

sndrs commented 7 years ago

do we have 3 issues here then?

but

does that leave anything out?

if so, they seem pretty much covered by @joelochlann's proposal?

I'm sure if we start converting to native we'd find there aren't many scenarios where it's much more effort to use native alternatives to the lib, and if we do come across them we can write our own helpers.

this is my gut feeling too. the only thing i think i'd be wary about is the crazy edge cases better used libraries would capture and handle for free.

by way of comparison, if we drop these 3 for a few minor helpers, we could still add preact as a preferred way to manage updates (if it is) while not requiring it, and save bytes:

$ npx package-size bean,qwery,bonzo preact

package           size     minified  gzipped
bean,qwery,bonzo  66.6 kB  22.2 kB   7.89 kB
preact            21.3 kB  8.2 kB    3.33 kB
regiskuckaertz commented 7 years ago

The question is whether the absence of complex DOM manipulation scenarios (there are a few) is because there is no need for it or because the amount of boilerplate is discouraging. All the querying, the testing and intermediate values for sometimes a very small impact.

What this calls for is a lens combinator library for the traversing part, and then the rest is handled by native APIs. 😃

sndrs commented 7 years ago

What this calls for is a lens combinator library

i think we can all agree on that!

(what is a lens combinator library btw?)

regiskuckaertz commented 7 years ago

😂 it is a set of composable functions for manipulating complex data structures. Remember the discussion where in JS it is tedious to access a property many levels deep in an object, without an existential operator? That is exactly the same problem, only more complicated, with a DOM tree. Lenses allow you to do that, it is sort of like XPath with read-write access. I've been thinking about it, and I'm convinced it is also a solution to completely bypass the virtual dom and the dom diffing algorithm.

gtrufitt commented 7 years ago

So when is Regact coming out?

joelochlann commented 7 years ago

Regact is THE framework of 2018

Pre-order your T-shirts now

regiskuckaertz commented 7 years ago

As if something truly interesting could ever not come from Facebook, right? 😉

SiAdcock commented 6 years ago

Another argument in microlibs' favour... Flow is brutal when you're using the vanilla DOM API. Especially in tests, where you find yourself having to null-check the results of queries that you know will find an element. Using the bonzo API is a lot more Flow-friendly, as its guaranteed chainability means you don't have null-check every call to .classlist, .innerHTML, .style, etc, etc

walaura commented 6 years ago

Test-wise it might be possible to wrap the native querySelector and querySelectorAll into a microlibrary that just returns type safe results but we might be better off just using bonzo

regiskuckaertz commented 6 years ago

... or jQuery. Bonzo is not maintained anymore, whereas jQuery still enjoys continuous care.

SiAdcock commented 5 years ago

I guess the push for componentisation has superseded the need for refactoring references to these libs. Shall we sunset this discussion?

SiAdcock commented 5 years ago

Closing this issue as no longer applicable to our client side modernisation strategy. Feel free to reopen if you can think of a reason to!