PDA

View Full Version : overriding / callParent behavior change (bug?)



maksimenko
1 Nov 2011, 3:33 PM
Hello there,

When overriding a constructor in ExtJS 4.1, the overridden constructor gets called when this.callParent() (instead of on this.callOverriden()) is used on the new function.



Ext.override(Ext.data.Store, {
constructor: function () {
......
this.callParent(); // this calls the overridden Ext.data.Store constructor instead of the Ext.data.AbstractStore constructor
....
}
});


Is this an intentional behavior change or an unnoticed bug?

The problem with this behavior is that if I try to override a constructor to provide custom functionality or fix a bug, the overridden constructor with the undesired code is getting called too, thus giving unexpected results...

Let me know if you need more info

mitchellsimoens
2 Nov 2011, 11:37 AM
Have you tried callOverridden()?

maksimenko
2 Nov 2011, 4:05 PM
Hello Mitchell,

Maybe I didn't explain myself correctly.

I don't want to call the overriden Ext.data.Store constructor, in fact, I expect it to not be called.

The problem is that, now in the 4.1 branch, with the changes made to the callParent function code, the this.callParent() is calling the overriden constructor and I expect it not to do so, instead I expect it to call the Ext.data.AbstractStore constructor...

Hope I explained myself better this time,

dongryphon
3 Nov 2011, 1:11 AM
Yes, this was changed as part of the Ext.define/override functionality. I will add it to the API changes page. If you haven't seen this yet, you might find it useful:



Ext.define('My.patch.Store', {
override: 'Ext.data.Store',

constructor: function () {
}
});


The override can then be "required" and dynamically loaded or brought into an optimized build. If an override is required (say 'My.patch.*'), it won't actually be used unless the target class is also used.

From a design perspective, overrides in 4.1 are almost like partial classes and will allow a single class to be implemented in multiple, independent pieces (rather than the problematic approach of "AbstractFoo" and "Foo" classes).

Is this need very common in your overrides?

To bypass the original method, in 4.1, you have to go old-school:



constructor: function () {
...
Ext.data.AbstractStore.prototype.constructor.call(this);
...
}

skirtle
3 Nov 2011, 8:07 AM
I only use overrides to patch bugs. It's pretty common to need to skip the buggy overridden method in such circumstances. Personally I always used the 'old-school' prototype approach when I was doing this anyway, relying on the callParent() magic always struck me as a bit risky when I'm subverting a class's original methods.

LesJ
3 Nov 2011, 8:28 AM
From a design perspective, overrides in 4.1 are almost like partial classes and will allow a single class to be implemented in multiple, independent pieces (rather than the problematic approach of "AbstractFoo" and "Foo" classes).

Is this need very common in your overrides?

Don, hi!

I use the "AbstractFoo" and "Foo" approach. Can you explain (in more detail) why this approach is problematic and how I could use overrides in 4.1?

Also, does it mean that classes such as AbstractComponent or AbstractContainer will be eliminated?

I still don't understand why for example there is the AbstractComponent class... why not fold its content into the Component class?

skirtle
3 Nov 2011, 8:49 AM
I believe AbstractComponent and AbstractContainer exist so that the code can be shared with Sencha Touch.

If I've understood what Don said correctly, I believe these classes will disappear because the new style of overrides allow this code to be reused without the need for the abstract classes. They are still there in 4.1-pr1 though.

mitchellsimoens
3 Nov 2011, 8:51 AM
Abstract classes are still a good idea IMO... Creating your own components sometimes you can just extend the abstract to accomplish what you need.

maksimenko
3 Nov 2011, 9:17 AM
Hello Don,

Thanks for your response.

As skirtle do, I mostly use overrides to patch bugs, so, in those cases I'd like the overridden method not to be called.

I've a few overrides in 4.0 and now I had to do a few for 4.1 so I could test the app ( and I've seen great perf. improvements in every browser... but still couldn't test on IE6 with a surface error :( )

Anyway, now I understand the reasoning behind the change. And the workaround of using the old school way of calling the superclass method works fine, so it's OK.

Thanks again for your response

dongryphon
3 Nov 2011, 11:03 AM
@LesJ,

The AbstractFoo/Foo approach can still be valid in many cases, don't get me wrong. Inheritance is still a Good Thing. :)

AbstractElement/Element and AbstractComponent/Component, however, were really "artifacts of implementation" where we were trying to share code between Touch and Ext JS. An app should never create an AbstractElement or AbstractComponent and really nothing other than Element or Component should even derive from them. This is not really what inheritance is all about, but it was the only tool at hand at the time. The problem is that this rippled through everything, including the docs, forcing these details to be in front of everyone.

It is possible that we will fold such things together in the future and maintain aliases for the old names. If we did this, the AbstractElement class would be renamed to Element and the current Element class would become an override.

dongryphon
3 Nov 2011, 11:05 AM
Abstract classes are still a good idea IMO... Creating your own components sometimes you can just extend the abstract to accomplish what you need.

True enough. Not all base classes are bad ... just some of them :)

maksimenko
3 Nov 2011, 11:06 AM
Hi all,

Well, after testing some cases, I'm really not liking this new behavior in Ext.override('...', {...});, maybe I'm still missing something,

For example, in 4.1, I have a few overrides like this one:


Ext.override('Ext.layout.container.Fit', {
calculate: function (ownerContext) {
var me = this,
childItems = ownerContext.childItems,
/* begin workaround */
//length = childItems.length,
length = childItems !== undefined ? childItems.length : 0,
/* end workaround */
targetSize = me.getContainerSize(ownerContext),
contentSize = { width: 0, height: 0 },
i;


for (i = 0; i < length; ++i) {
me.fitItem(ownerContext, childItems[i], targetSize, contentSize);
}


if (!ownerContext.setContentSize(contentSize.width, contentSize.height)) {
me.done = false;
}
}
});


but when Ext.layout.Context.runLayout runs, it calls layout.calculate(ownerContext), and, for a fit layout, the original calculate function is called instead of the override and thus it's giving me:
"Uncaught TypeError: Cannot read property 'length' of undefined"... (This error happens when there is a panel without child items, just using the html: property, I can report it in another post if it helps)

I understand and see the value of being able to have partial definitions of classes (coming from a .net world where partial classes are used somewhat widely), but maybe using another name instead of override, and let override do what the name suggest...

Thanks again

mitchellsimoens
3 Nov 2011, 11:07 AM
True enough. Not all base classes a bad ... just some of them :)

Can't disagree with that!

dongryphon
3 Nov 2011, 11:09 AM
I believe AbstractComponent and AbstractContainer exist so that the code can be shared with Sencha Touch.

True.


If I've understood what Don said correctly, I believe these classes will disappear because the new style of overrides allow this code to be reused without the need for the abstract classes. They are still there in 4.1-pr1 though.

They could, but not likely in 4.1. If we did, the AbstractComponent/Container names would become alternate names to maintain compatibility.

dongryphon
3 Nov 2011, 11:15 AM
when Ext.layout.Context.runLayout calls layout.calculate(ownerContext) for a fit layout, the original calculate function is called and is giving me:
"Uncaught TypeError: Cannot read property 'length' of undefined"... (This error happens when there is a panel without child items, just using the html: property, I can report it in another post if it helps)


Yes, that would help (in a new thread). The workaround in your override is attacking the symptom of a deeper problem. The childItems array should never be null, so there is something much more interesting afoot here.

dongryphon
3 Nov 2011, 11:20 AM
Well, after testing some cases, I'm really not liking this new behavior in Ext.override('...', {...});, maybe I'm still missing something,

I understand and see the value of being able to have partial definitions of classes (coming from a .net world where partial classes are used somewhat widely), but maybe using another name instead of override, and let override do what the name suggest...

Override works as it did in 4.0, but callParent did change in 4.1. Is that the part you are not liking? Just curious because your example does not call the overridden method.

maksimenko
3 Nov 2011, 11:33 AM
Yep, I know that workaround should not be needed, but I'm doing those overrides so I can test the 4.1.

I've used a few overrides for fixing problems that I got in 3.x, then in 4.0 and for sure, somewhere in the future I'd need some overrides to fix bugs until they are fixed in new releases (hopefully we'll be getting access to the 4.1 svn/git branch soon), so I need to find a reliable way to override some methods in ExtJS >= 4.1.

I've tried modifying directly ext-all-dev and they let me use my app with a few quirks here and there, but no errors, and I'm very pleased with the performance improvements, but obviously this method shouldn't be used....

I've tried doing the old school "Ext.layout.container.Fit.prototype.calculate = function (ownerContext) { ... };" for all my overrides, and although no error is raised, the app is not rendering as it is if I do the changes directly in ext-all-dev.... gotta investigate more why.

So, that's why I'm raising this issue about Ext.override(''..", {..}); these kind of overrides worked on 3.x and 4.0, so, I'm trying to find a way to be able to override core methods reliably.

maksimenko
3 Nov 2011, 11:40 AM
Override works as it did in 4.0, but callParent did change in 4.1. Is that the part you are not liking? Just curious because your example does not call the overridden method.

Hello Don, thanks for your response,

Yup, in my tests, the override doesn't seems to be called, so that's the part I'm not liking (as I'm guessing more changes where done to that behavior, but maybe I'm wrong - I've not checked the core code to verify it -), I've no problem with the callParent change...

My override isn't getting called (tried some console.log) when the runLayout calls layout.calculate(....) as it calls the overridden original method...

dongryphon
3 Nov 2011, 11:49 AM
Yep, I know that workaround should not be needed, but I'm doing those overrides so I can test the 4.1.

Understood. Still it would help if you could pass along whatever problems you are running into. It might be less investment on your part when the problems are too deep to just patch over.

Thanks for digging in on the release!

dongryphon
3 Nov 2011, 11:58 AM
Yup, in my tests, the override doesn't seems to be called, so that's the part I'm not liking (as I'm guessing more changes where done to that behavior, but maybe I'm wrong - I've not checked the core code to verify it -), I've no problem with the callParent change...

My override isn't getting called (tried some console.log) when the runLayout calls layout.calculate(....) as it calls the overridden original method...

The form you have should work in both 4.0 and 4.1, give or take using the string class name. You could try removing the quotes on the class name you are overriding.

You could try (in 4.1 only):



Ext.define('My.patch.Fit', {
override: 'Ext.layout.container.Fit',

calculate: function () {
...
}
});

//--- in your app:

{
layout: 'fit',
requires: 'My.patch.Fit'
}


With appropriate namespaces and pathing of course. :)

This is a big part of why Ext.define/override was added in 4.1: to help manage overrides and when they are included in a build and when they can be (safely) applied to their target class.

Hope this helps!

maksimenko
3 Nov 2011, 12:31 PM
Hello Don,

Thanks again for your response,

That helped!

Using the following code worked perfectly and I'm able to test the app running in 4.1 (I've changed all my overrides to this form)


Ext.define('MM.patch.layout.container.Fit', {
override: 'Ext.layout.container.Fit',

calculate: function (ownerContext) {
console.log('override called');
var me = this,
childItems = ownerContext.childItems,
/* begin workaround */
//length = childItems.length,
length = childItems !== undefined ? childItems.length : 0,
/* end workaround */
targetSize = me.getContainerSize(ownerContext),
contentSize = { width: 0, height: 0 },
i;


for (i = 0; i < length; ++i) {
me.fitItem(ownerContext, childItems[i], targetSize, contentSize);
}


if (!ownerContext.setContentSize(contentSize.width, contentSize.height)) {
me.done = false;
}
}
});


But just to let you know, using the following code (the < 4.1 way, without the Ext.define('..', { override: ... });) don't...


Ext.override('Ext.layout.container.Fit', {
calculate: function (ownerContext) {
console.log('override called');
var me = this,
childItems = ownerContext.childItems,
/* begin workaround */
//length = childItems.length,
length = childItems !== undefined ? childItems.length : 0,
/* end workaround */
targetSize = me.getContainerSize(ownerContext),
contentSize = { width: 0, height: 0 },
i;


for (i = 0; i < length; ++i) {
me.fitItem(ownerContext, childItems[i], targetSize, contentSize);
}


if (!ownerContext.setContentSize(contentSize.width, contentSize.height)) {
me.done = false;
}
}
});


Don't know if it should be filed as a bug or not... what's even weirder is that I had an override in this old form for the ComboBox constructor and it worked fine there...

Now that I understand the new way, I quite like it, it's cleaner and easier to include/exclude on builds.

Thanks again for your help throughout this issue

dongryphon
3 Nov 2011, 12:44 PM
But just to let you know, using the following code (the < 4.1 way, without the Ext.define('..', { override: ... });) don't...


Ext.override('Ext.layout.container.Fit', {
calculate: function (ownerContext) {
console.log('override called');
var me = this,
childItems = ownerContext.childItems,
/* begin workaround */
//length = childItems.length,
length = childItems !== undefined ? childItems.length : 0,
/* end workaround */
targetSize = me.getContainerSize(ownerContext),
contentSize = { width: 0, height: 0 },
i;


for (i = 0; i < length; ++i) {
me.fitItem(ownerContext, childItems[i], targetSize, contentSize);
}


if (!ownerContext.setContentSize(contentSize.width, contentSize.height)) {
me.done = false;
}
}
});


Don't know if it should be filed as a bug or not... what's even weirder is that I had an override in this old form for the ComboBox constructor and it worked fine there...

Now that I understand the new way, I quite like it, it's cleaner and easier to include/exclude on builds.

Thanks again for your help throughout this issue

Glad I could help. :) Try this in the pre-4.1 version:



Ext.override(Ext.layout.container.Fit, {
calculate: function (ownerContext) {
console.log('override called');
var me = this,
childItems = ownerContext.childItems,
/* begin workaround */
//length = childItems.length,
length = childItems !== undefined ? childItems.length : 0,
/* end workaround */
targetSize = me.getContainerSize(ownerContext),
contentSize = { width: 0, height: 0 },
i;

for (i = 0; i < length; ++i) {
me.fitItem(ownerContext, childItems[i], targetSize, contentSize);
}

if (!ownerContext.setContentSize(contentSize.width, contentSize.height)) {
me.done = false;
}
}
});


The quoted class name is I think the problem.

maksimenko
3 Nov 2011, 12:58 PM
Yup, that was it!,

I should have copy/pasted an Ext.define('...', {}); and changed the define to override.... because the one that were working and all in 3.4 and 4.0 are without quotes....

Thanks again for your help!

dongryphon
3 Nov 2011, 7:43 PM
Glad to hear that solved it.

Curtis Fletcher
3 Jul 2012, 3:41 AM
So am I right in assuming that there is _no_ way to dynamically patch an ext object if the method happens to use callParent?

For example, There is behaviour within Ext.grid.column.Column.initComponent that I want to change (sorting is switched off for grouped columns, which is unnecessary when sorting is done remotely), not on a case-by-case basis but globally, even if the columns are created by ext-internal code. Normally I would use Ext.override but that breaks in 4.1 due to callParent calling the overridden initComponent.

skirtle
5 Jul 2012, 3:12 PM
So am I right in assuming that there is _no_ way to dynamically patch an ext object if the method happens to use callParent?

I'm not sure this question makes sense. If you use callParent in an override it will call the overridden method. If you don't want to call that method, don't use callParent. You can still call implementations further up the prototype chain by referencing them directly just like you always could.

Curtis Fletcher
6 Jul 2012, 12:27 AM
I'm not sure this question makes sense. If you use callParent in an override it will call the overridden method. If you don't want to call that method, don't use callParent. You can still call implementations further up the prototype chain by referencing them directly just like you always could.

I'm not choosing to use callParent. I'm replacing an ext method that needs to use callParent to use functionality in it's parent class.

Full example:
Grouped grid columns explicitly get "sortable" set to false in Ext.grid.column.Column.initComponent this behaviour is unnecessary if "remoteSort" is set, hence I wan't to change it globally. Previous to Ext 4.1 I would replace Ext.grid.column.Column.initComponent with a version that behaved as I expected using override. If "callParent" was used it would call the method in the parent class not the original Ext.grid.column.Column.initComponent

tl;dr override used to be usable to "patch" ext without messing with inheritance, now it can't. What can I use to so the same thing that override used to do?

dongryphon
6 Jul 2012, 12:38 AM
Hi @curtis,

I am not really following what you are describing, sorry. If you have overridden initComponent, how can the original initComponent (the one with callParent you mentioned) be getting called if you are not using callParent or callOverridden?

It would be best if you would post the content of your override. At least the important parts: the Ext.override (or Ext.define/override) call and the initComponent in your override.

Curtis Fletcher
6 Jul 2012, 1:03 AM
Apologies for failing to explain this. I am using callParent, because I _must_ use it.

I need to replace initComponent in Ext.grid.column.Column. with a version that is only subtly different.
initComponent _must_ use callParent to invoke Ext.grid.header.Container.initComponent (and so on) hence my replacement must use callParent

E.G.

Ext.define('Ext.x.child', {
extend: 'Ext.x.base',
initComponent: function() {
#complex init here
#code that makes problematic assumptions here
#more complex init here
this.callParent()
#more complex init here
}
}


The call chain was Ext.x.child.initComponent->Ext.x.base.initComponent


Previously I could



Ext.override(Ext.x.child, {
initComponent: function() { //overridded
#complex init here
#FIXED ASSUMPTION CODE
#more complex init here
this.callParent()
#more complex init here
}
}


The call chain was Ext.x.child.initComponent//overridden->Ext.x.base.initComponent


Now the callchain is Ext.x.child.initComponent//overridded->Ext.x.child.initComponent->Ext.x.base.initComponent

skirtle
6 Jul 2012, 4:01 AM
I still don't understand why you think you must use callParent. callParent is just a convenience method for invoking a method on the superclass. There's nothing to stop you calling the method you want directly, like this:


initComponent: function() {
// Complex init

Ext.grid.column.Column.superclass.initComponent.call(this);

// Complex init
}