Lessons learnt from a bug

This is a rant about a bug report I raised with ExtJS a few weeks ago. That said, I’m using the bug more as a teachable moment than anything else; I’m certainly not trying to bag ExtJS (which I quite like, despite some of its quirks). But this bug does highlight a number of “things done wrong”, which I want to learn from so that I don’t commit the same errors.

(No knowledge of ExtJS is required, and whilst I will describe the details of the bug in depth, the technical issues involved aren’t meant to be the takeaway points)

Background Reading

Okay, so let’s start with some background. ExtJS is a “code-centric” JavaScript/Web UI library. You use it to build web-based interfaces predominately using JavaScript (with some HTML templating), as opposed to, say, JQuery, which mainly works by adding behaviour to a HTML page. It’s definitely in the “advanced” category of tools, and comes complete with a rich set of UI components. The bug in question is with one of these components.

The components themselves have an extensive set of CSS styles that determine how they look when rendered. These CSS styles are organised into themes, which are written using Sass – a pre-processor for writing CSS. The themes take advantage of a lot of Sass features, especially inheritance and variables. The Sass themes are themselves partitioned by the ExtJS components – there is a Sass file per ExtJS component (this is used during packaging to only include the CSS rules for the classes you use, resulting in a smaller file for users to download). You can also make your own theme, by extending one of the defaults and customising to suit.

Within a theme, you can provide multiple styles for a component. This lets you render the same type of component different ways, using a single parameter – the ui parameter. The value of this parameter (which defaults to ‘default’) is used as a modifier  for the CSS classes of the generated HTML DOM nodes that are ultimately shown, which in turn should have matching CSS generated via the SASS themes.

One of the components that is shipped with ExtJS is a tab panel (you can see what it looks like via that link). The bug is with this class.

The details of the bug

It’s really simple: in ExtJS 4.2.0, if you set the ui parameter on a tab panel, you’d get the appropriate style on the tab panel itself, and on DOM nodes directly created by the tab panel, but not on DOM nodes created by components within the tab panel – e.g. the tab buttons, and the contents of the tab cards themselves.

This is not the case in ExtJS 4.2.1. And the consequences on customised uis is quite impressive.

As part of the bug report, I knocked up a quick example (you can get it from the bug report itself if you want). It simply renders two tab panels side-by-side; the one of the left uses the default UI, and the one on the right uses a custom UI that simply adds a red border.

Here’s the before picture (with ExtJS 4.2.0):

and here’s the after picture (with ExtJS 4.2.1):

Quite a dramatic difference. This happens because the CSS class on the TabBar (the div that holds the buttons) in the second tab changed from being ‘x-tab-bar-default’ to ‘x-tab-br-highlight-framed’ (as the name of the ui I set was ‘highlight-framed’). Yet I didn’t create a custom UI for the TabBar component – just the TabPanel (actually, I created a custom ui for the Ext.panel.Panel class – the parent class for the Tab Panel).

For some reason, the Powers-That-Be at Sencha declared this not to be a bug, saying it was the intended behaviour. Here starteth the lesson.

Lessons Learnt

It Really Was A Bug

This was a large change in behaviour that was not mentioned in the release notes. That means there was a bug in here somewhere.

If the old behaviour was incorrect, and the behaviour in 4.2.1 is what they intended all along, then there should have been a bug for that which was referenced in the release notes.

If the old behaviour was correct, and Sencha has changed their mind, then a change of this sort of impact should be mentioned in the release notes and not included in a ‘bug fix’ release (which a ‘z-level’ release is). Bug fix releases are for fixing deviations from the intended behaviour of the original release, not to introduce changes. Deliberate changes to what is intended should be left for x- or y-level releases. Oh, and mentioned in the release notes.

Finally, this behaviour is not mentioned in the documentation at all. In fact, there is no special mention of the ‘ui’ property for either the Ext.tab.Panel or Ext.tab.Bar classes – they merely inherit the property from the base AbstractComponent class. Even if this was a deliberate change, and was mentioned in the release notes, the lack of documentation of this behaviour is itself a bug.

Lesson one: don’t say something isn’t a bug when it is. All that achieves is to annoy your customer – the one who is helpfully trying to provide feedback to improve your product.

Consistency Is Important

ExtJS provides lots of components – several dozen at least, if not over a hundred. Many of these use internal components to provide functionality; these internal components aren’t intended to be managed by the user.

The behaviour of the ui parameter is not consistent between these components. Some pass changes to the ui parameter down to the internal component, and some do not. Not one of these classes documents this behaviour, so the only way to find out is trial and error.

Sencha needs to decide what the rule should be: either the ui parameter from a component is passed down to internal components, or its not. This rule should then be documented. When it is necessary to deviate from that rule, then that deviation itself should be documented.

(FWIW, the ui component is never passed down to user-managed components)

Lesson two: be consistent. Probably the best takeaway from the ‘convention-over-configuration’ movement has been increased consistency.

Make Workarounds Easy

It is not unreasonable to expect that someone might want to provide a custom component ui for a component that has internal components. These custom uis may range from as simple as changing a colour or border, all the way up to providing a complete replacement, including background and control images, etc. This task should not be any harder than it needs to be.

In order to use a custom ui for an Ext.panel.Panel inside an Ext.tab.Panel, it is necessary to provide Sass definitions that inherit from the base theme and do nothing for the Ext.tab.Panel, Ext.tab.Bar and Ext.tab.Tab classes. If you don’t define them you get the result shown above – missing styles that result in incorrect rendering.

However, even this isn’t enough. It turns out that, for the standard ExtJS themes people are likely to extend, the controls in the tab itself (like the close button) are done as images. The Sass definition that defines these styles uses the ui name as part of the image name. So you can’t simply extend the base ui – you need to either redefine the code for the images completely, or you have to create your own images.

If the user is expected to extend the theme for the Ext.tab.Tab, the attribute used for determining the image should be configurable so that it’s easy to reset to the default. Some of the component uis are done like this – but not all.

Making minor tweaks to a custom UI has become a lot harder in ExtJS 4.2.1 because of this. The one bonus is that it is now possible to define a complete custom UI for a tab, including changing images – because supposedly that wasn’t possible before (or at least that’s what Sencha told me in response to my service request).

Fortunately, there is a reasonably easy workaround to this. Sencha didn’t tell this to me, though – I had to work it out for myself. And they haven’t confirmed that it is the intended workaround either, so take this with a grain of salt.

Instead of defining the tab panel like this:

{ xtype: 'tabpanel',
  flex: 0.5,
  ui: 'highlight-framed',
  items:[
    { title: 'Framed Tab 1', closable: false },
    { title: 'Framed Tab 2', closable: true }
  ]
}

you can define it like this (the relevant change is line 4):

{ xtype: 'tabpanel',
  flex: 0.5,
  ui: 'highlight-framed',
  tabBar: { ui: 'default' },
  items:[
    { title: 'Framed Tab 1', closable: false },
    { title: 'Framed Tab 2', closable: true }
  ]
}

This overrides the ui for the internal Ext.tab.Bar component and sets it back to the default (and, as a consequence, the internal Ext.tab.Tab components).

The real kicker here is that the same workaround could have been used to provide a fix to the original problem that triggered the behaviour change. If Sencha had chosen that approach, then only people who are trying to define a custom UI for a tab bar would need to do anything, and people merely trying to style the panel wouldn’t have to be pushing in changes as a result of an upgrade.

Lesson three: Make workarounds for simple problems simple.
Corollary: Don’t let the edge case make the common case hard.

Keep Internal Components Private

Another lesson that was reinforced for me is that you shouldn’t be exposing implementation details to users of your API.

The Ext.tab.Tab and Ext.tab.Bar classes are exactly that – implementation details. They are even documented as such. The user doesn’t make them – they are instantiated by the Ext.tab.Panel as needed.

In a way, then, the fact that the ui parameter is passed down really is the correct behaviour. The only thing is… why should it be necessary? Why should the client developer have to create a custom UI component for an internally managed component?

All of the styles for Ext.tab.Bar and Ext.tab.Tab could – and should – have been defined and managed within the style for Ext.tab.Panel. It just shouldn’t be necessary for the client developer to extend those component uis at all; they shouldn’t even exist.

It’s not like there’s a one-for-one correspondence with components and DOM nodes, anyway. A component usually has several nodes, each with an appropriate classification on the CSS class name. Adding a couple more managed at the Ext.tab.Panel level wouldn’t have been hard to do. Heck, using mixins, they could still have kept those definitions in distinct files if they wanted.

Lesson Four: Don’t make clients of your API prisoners of your implementation choices.

Don’t Repeat Mistakes

This one is more of a prediction of what is going to be learnt than a lesson already learnt. ExtJS 4.2.2 is due soon – the release date is expected to be September 12, which is later this week. I somehow doubt that the problems highlighted in this post – the lack of documentation, the lack of consistency, the difficulty of the workarounds, or the continued reliance on the internal implementation – will have been corrected. Nor have I been promised as such – all I have been told is that an enhancement request was filed on my behalf.

(This prediction is not at all based 100% on having examined the recent nightly builds. That would be, like, cheating or something)

Lesson Five: Learn from your mistakes, don’t repeat them.

Conclusion

Let’s wrap this up, then…

• As I said at the start, I’m not trying to pick on ExtJS or Sencha – I’m actually quite impressed by the product, and I found the upgrade for 4.2.0 and 4.2.1 to be quite smooth (this was one of only three bugs we found).
• This bug, while fairly minor and with an easy workaround, highlighted a number of systematic issues – the lack of consistency in particular. Consistency in API design is extremely important – pick one way and do it like that always (or as nearly always as feasible). Being consistent is more important than trying to be 100% correct.
  In many ways, it doesn’t matter wether internal components have their UIs managed by their parents or not. But it does matter that they should all follow the same rule – and in the course of investigating this bug I found at least three variations (‘inherit from parent’, ‘don’t inherit from parent’ and ‘look at a different property on parent to inherit from’ – the latter occurs at least twice with different property names, but I’m only counting that as one variation)
• Lack of documentation bites. Especially when your behaviour isn’t consistent.
• Not mentioning the change in the release notes is a huge cardinal sin. Changes that impact the user of your API must be mentioned.

Anyway – I hope you all enjoyed this rant. I personally found it quite therapeutic to write – it took me nearly a week to stop getting seriously annoyed every time I thought about the problem. Still, it beats writing about politics…