PDA

View Full Version : [RANT] Patch release code base changes



ThorstenSuckow
29 Sep 2009, 1:48 PM
I'm currently updating my application's code to work with 3.0.2 (updating from 3.0.0) and I have to admit that I'm a little bit disappointed about the way changes find their way into a patch release.
From a coders point of view, I would expect crucial bug fixes in a patch release but have yet found changes in the API that make updating my code (user extensions, too) a pita.
Then, there seem to be some odd changes that make my app not behave as before - I haven't touched any private/protected functions in the code, just playing with config properties.

I'd expect my app to behave the same way as it did with the point release (except for the bugs, of course ;) ), but right now I'm afraid I'll rather skip patch releases to save me some time.

Anyone else experienced the same?

hendricd
29 Sep 2009, 2:14 PM
Indeed! 8-|

abe.elias
29 Sep 2009, 2:47 PM
Hey Thorsten and Doug,

It's obviously not our intention to frustrate you guys. Can you provide some more details so we can correct it going forward. It's our second patch release and we'd like to catch our missteps early on.

Thx,
~A

ThorstenSuckow
29 Sep 2009, 2:59 PM
Hi Abe,

no crucial bugs introduced, yet, AFAICS.

However, I have the feeling that there should be a better management regarding branching point releases and keeping the main dev branch for crucial bug fixes, which get then merged into the point release branch later on as I have the feeling the patch releases already include important changes which pave the way for features that will be available in the point release.

I know branching can be a hazzle if not planned right, but keeping in mind that there are large apps out there where devs see a patch release as a "bug fix" only, this should maybe be a point worth discussing with the core devs.

evant
29 Sep 2009, 4:48 PM
There were a few minor changes to the API, however none of them should be "breaking". For example a new trackLabels config option, which defaults to false (so that the old behaviour is maintained) was introduced to allow tracking of hiding/showing/removing of form labels along with the component. It would be helpful if you could point out the areas where there have been some changes that break certain functionality.

abe.elias
29 Sep 2009, 5:08 PM
Here's some information to shed some light on what we are trying to accomplish with our patch releases.



No enhancements (esp that break your code)
No PUBLIC API changes
No NEW bugs... (hopefully!)


If we are not living up to that, we need and will address it. See http://www.extjs.com/products/releases.php for more details.

We have a staging branch which accepts patch commits and several internal sanity checks when preparing our patch releases. This is radically different from 2.x.x releases, that were "just another snapshot of trunk". So if something is falling through the cracks, lets find it and address it ASAP.

mjlecomte
29 Sep 2009, 7:06 PM
From a coders point of view, I would expect crucial bug fixes in a patch release ...
I'd expect my app to behave the same way as it did with the point release (except for the bugs, of course ;)
This is the intent for patch releases.




but have yet found changes in the API that make updating my code (user extensions, too) a pita.

Then, there seem to be some odd changes that make my app not behave as before

This is NOT the intent for patch releases.

As said above, could you describe in more detail your unexpected experience or PM code or link to app for further inspection?

The patch releases are only intended to address bugs. Some fixes end up including some changes to the API which are deemed appropriate by the core development team to effectively address an issue.

hendricd
29 Sep 2009, 8:36 PM
Hey Thorsten and Doug,

It's obviously not our intention to frustrate you guys. Can you provide some more details so we can correct it going forward. It's our second patch release and we'd like to catch our missteps early on.

Thx,
~A

@Abe, Team
It would be counter-productive to fully analyze all the recent SVN logs in detail, but one representative example stands out (from my standpoint for ManagedIframe, ux.Media, ux.Media.Charts):

Add: Ext.Component :: contentEl, html config options
Add: Ext.BoxComponent :: autoScroll
Remove: contentEl, html, autoScroll from Ext.Panel

Were these class heirarchy changes introduced to solve a bug or were they 'nice-to-haves'? Although cool, surely, these could have waited for 3.1?

Although the actual fallout (for me) from these was minimal, I almost had a heart-attack :s when I saw them. It did require a full class review/test cycle of several of these popular ux's to assess the impact.

: And I [s]never want to will not go through what I did for Ext 2 -> 3 again -- there were significant rewrites (for me) involved just 2 weeks before you released Ext3 -- I had no idea what was coming. For me, that (combined with privates-functions-closures-from-hell) was a significant out-of-pocket expen$e that could have been avoided! >:)

It's a difficult problem to solve. Innovation is cool, but we should find a way for the x-ext-salaried to convey to the x-open-source community what's coming (especially when you've got a Marketplace coming to life soon -- which breeds another set of responsibilities).

It could mean:


scrutiny about what is truly a fix vs enhancement (who makes that decision?)
assessing the impact on internal/community ux's (again, who makes that decision?)
publishing the 'change log/TRAC list' some time before a point release. Something you're currently not accustomed to or yet-willing to give public access to.


Dunno, just thinking out loud... :-?

evant
29 Sep 2009, 9:36 PM
Doug, not sure what you mean by the autoScroll changes. These were not applied to the patch branch, they are waiting for 3.1.

ThorstenSuckow
30 Sep 2009, 12:49 AM
I'll go through my repository asap and list a few issues that were bothering me. For a start:
- http://www.extjs.com/forum/showthread.php?t=81555.
- There where changes made to the "scrollOffset" property in the GridView
- Some other changes that prevent the livegrid from being rendered in IE (I know I touch a lot of private impl there so no offense, but it worked in 3.0.0 and I expected it to work in 3.0.2), I haven't had the time to look into it in detail yet
- The menu in the GridViewMenuPlugin gets aligned to the wrong side when rendered first, after that it shows correct

One thing about the "privat" stuff is, that there is sometimes no other way to implement custom logic into an ux than to touch those methods. Marked as private for the (core) devs, I sometimes find myself overriding those methods because important accessors are missing (for example, url property in the Ext.Direct-API). This is not because I do not know how to OOP a component, it's more because some parts of the API are not very override-friendly:
Take the Ext.lib.Ajax-singleton for an example. It's crucial for me and the apps I'm working on to attach a queue mechanism to it, but since 3.0.0 it got worse due to the pattern used therin. Implementing a queue means to copy&paste the whole source, and diff it against new releases once they hit the road.

Enough nitpicking for now. Other than that, I'm still in love with the work you guys are doing! ~o)

evant
30 Sep 2009, 3:00 AM
@MindPatterns

1) This was one that slipped through, probably shouldn't have gone in the patch release.
2) While this introduced a new method, it should not have broken any existing code. The scroll offset would still be honoured if specified by the user. If not it will use the method to automatically calculate the size. This was a bug fix, since the scroll bar width differs depending on the visual style running on the OS (try WinXP with extra large fonts. Yikes!).

Regarding the points about private methods, it's difficult to manage this. If a method is marked private the implementation may change (or it may not even exist) at any point in time, patch release or not. We understand that people can and do override these methods, however if you do this, it's "at your own peril", so to speak.

If we can't change the private methods, we can't really modify much for the patch releases!

That being said, your comment regarding certain things not being extension friendly is taken on board and we are looking to refactor certain parts to make this easier to do. For example, removing anonymous functions inside classes to private methods.

mjlecomte
30 Sep 2009, 4:14 AM
[B]Add: Ext.Component :: contentEl, html config options
Add: Ext.BoxComponent :: autoScroll
Remove: contentEl, html, autoScroll from Ext.Panel

Were these class heirarchy changes introduced to solve a bug or were they 'nice-to-haves'? Although cool, surely, these could have waited for 3.1?



Did I miss this? Seems to me none of these were applied to the patch branch.

As to the side rant of ext core privacy I believe that is being addressed to be reverted. But I think that should go to the other related thread or new thread to keep this on topic about the patch branch content.

hendricd
30 Sep 2009, 5:39 AM
Did I miss this? Seems to me none of these were applied to the patch branch.

As to the side rant of ext core privacy I believe that is being addressed to be reverted. But I think that should go to the other related thread or new thread to keep this on topic about the patch branch content.

@Mj -- you're right (and didn't miss much). I didn't study every branch and trunk before leaping on that particular case. ;)

But, the point still stands: How would we ux-providers get notice of a future change like that, today (in order to assess the future impact of them)? If I didn't watch my CommitMonitor like a hawk, I surely would have missed it, and as we're seeing now, others. :-?

evant
30 Sep 2009, 6:34 AM
You'll notice that we've added tags for each commit (for example, fix, doc, changed, added).

In general, if it's added/changed/refactored, it might be something you'll want to check out.

abe.elias
30 Sep 2009, 6:46 AM
We're listening. We need more transparency for what's coming down the pipe so you don't have to watch CommitMonitor like a hawk. We can work on that.

If you and others are monitoring trunk you are getting the wrong impression of what we are trying to accomplish with the patch release and I fully understand your apprehension.


@Thorsten
We thought about having several branches and then merging them back and keeping trunk stable. It was discussed at length internally. The overwhelming thought was that we've all worked at organizations that had this approach and we hated it. We don't think what we have now is perfect, but its a huge improvement.

jay@moduscreate.com
30 Sep 2009, 6:54 AM
I feel bad about the autoScroll change. I asked for that one :(

hendricd
30 Sep 2009, 8:16 AM
Ah, shake it off guys.

Just need to organize a communication medium. We'll get there. ;)